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

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

[Charles Keepax]

Another round of SDCA fixes, a couple of fixes to the IRQ handling from
Maciej, a couple of fixes to the IRQ cleanup from Richard, and a minor
tweak to the IRQ handling from me.

Thanks,
Charles

Charles Keepax (1):
  ASoC: SDCA: Tidy up irq_enable_flags()/sdca_irq_disable()

Maciej Strozek (2):
  ASoC: SDCA: Fix overwritten var within for loop
  ASoC: SDCA: mask Function_Status value

Richard Fitzgerald (2):
  ASoC: SDCA: Fix cleanup inversion in class driver
  ASoC: SDCA: Unregister IRQ handlers on module remove

 include/sound/sdca_interrupts.h      |  4 ++--
 sound/soc/sdca/sdca_class.c          | 27 ++++++++++++++-------------
 sound/soc/sdca/sdca_class_function.c | 10 +++++++++-
 sound/soc/sdca/sdca_interrupts.c     | 23 +++++++++--------------
 4 files changed, 34 insertions(+), 30 deletions(-)

-- 
2.47.3

[PATCH 1/5] ASoC: SDCA: Fix overwritten var within for loop

[Charles Keepax]

From: Maciej Strozek <mstrozek@opensource.cirrus.com>

mask variable should not be overwritten within the for loop or it will
skip certain bits. Change to using BIT() macro.

Fixes: b9ab3b618241 ("ASoC: SDCA: Add some initial IRQ handlers")
Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_interrupts.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 1838dabcdf604..9acb0be84674c 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -117,9 +117,7 @@ static irqreturn_t function_status_handler(int irq, void *data)
 
 	status = val;
 	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
-		mask = 1 << mask;
-
-		switch (mask) {
+		switch (BIT(mask)) {
 		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
 			//FIXME: Add init writes
 			break;
-- 
2.47.3

[PATCH 2/5] ASoC: SDCA: mask Function_Status value

[Charles Keepax]

From: Maciej Strozek <mstrozek@opensource.cirrus.com>

According to the SDCA specification [1], when writing Function_Status during
handling this control, the value should mask off bit 7.

[1] MIPI Specification for SoundWire Device Class for Audio, version
    1.1, section 7.14.1.3 (Host Software Handling of Function_Status)

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 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 9acb0be84674c..0693209ffed1b 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -138,7 +138,7 @@ static irqreturn_t function_status_handler(int irq, void *data)
 		}
 	}
 
-	ret = regmap_write(interrupt->function_regmap, reg, val);
+	ret = regmap_write(interrupt->function_regmap, reg, val & 0x7F);
 	if (ret < 0) {
 		dev_err(dev, "failed to clear function status: %d\n", ret);
 		goto error;
-- 
2.47.3

[PATCH 3/5] 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>
---
 sound/soc/sdca/sdca_class.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 7af4e5d1b347f..732737f969891 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -137,6 +137,15 @@ 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;
+
+	cancel_work_sync(&drv->boot_work);
+
+	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 +166,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 +182,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,10 +235,6 @@ 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;
-- 
2.47.3

[PATCH 4/5] ASoC: SDCA: Unregister IRQ handlers on module remove

[Charles Keepax]

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

Ensure that all interrupt handlers are unregistered before the parent
regmap_irq is unregistered.

sdca_irq_cleanup() was only called from the component_remove(). If the
module was loaded and removed without ever being component probed the
FDL interrupts would not be unregistered and this would hit a WARN
when devm called regmap_del_irq_chip() during the removal of the
parent IRQ.

Fixes: 4e53116437e9 ("ASoC: SDCA: Fix errors in IRQ cleanup")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_interrupts.h      |  4 ++--
 sound/soc/sdca/sdca_class_function.c | 10 +++++++++-
 sound/soc/sdca/sdca_interrupts.c     |  7 +++----
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index b47003c3d26ef..a515cc3df0971 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -83,8 +83,8 @@ int sdca_irq_populate_early(struct device *dev, struct regmap *function_regmap,
 int sdca_irq_populate(struct sdca_function_data *function,
 		      struct snd_soc_component *component,
 		      struct sdca_interrupt_info *info);
-void sdca_irq_cleanup(struct sdca_function_data *function,
-		      struct snd_soc_component *component,
+void sdca_irq_cleanup(struct device *dev,
+		      struct sdca_function_data *function,
 		      struct sdca_interrupt_info *info);
 struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
 					      struct regmap *regmap, int irq);
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 61b725cd52056..31fc08d513077 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -203,7 +203,7 @@ static void class_function_component_remove(struct snd_soc_component *component)
 	struct class_function_drv *drv = snd_soc_component_get_drvdata(component);
 	struct sdca_class_drv *core = drv->core;
 
-	sdca_irq_cleanup(drv->function, component, core->irq_info);
+	sdca_irq_cleanup(component->dev, drv->function, core->irq_info);
 }
 
 static int class_function_set_jack(struct snd_soc_component *component,
@@ -417,6 +417,13 @@ static int class_function_probe(struct auxiliary_device *auxdev,
 	return 0;
 }
 
+static void class_function_remove(struct auxiliary_device *auxdev)
+{
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	sdca_irq_cleanup(drv->dev, drv->function, drv->core->irq_info);
+}
+
 static int class_function_runtime_suspend(struct device *dev)
 {
 	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
@@ -569,6 +576,7 @@ static struct auxiliary_driver class_function_drv = {
 	},
 
 	.probe		= class_function_probe,
+	.remove		= class_function_remove,
 	.id_table	= class_function_id_table
 };
 module_auxiliary_driver(class_function_drv);
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 0693209ffed1b..5cdabf8ae9da3 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -555,17 +555,16 @@ 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.
  * @function: Pointer to the SDCA Function.
- * @component: Pointer to the ASoC component for the Function.
  * @info: Pointer to the SDCA interrupt info for this device.
  *
  * Typically this would be called from the driver for a single SDCA Function.
  */
-void sdca_irq_cleanup(struct sdca_function_data *function,
-		      struct snd_soc_component *component,
+void sdca_irq_cleanup(struct device *dev,
+		      struct sdca_function_data *function,
 		      struct sdca_interrupt_info *info)
 {
-	struct device *dev = component->dev;
 	int i;
 
 	guard(mutex)(&info->irq_lock);
-- 
2.47.3

[PATCH 5/5] 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>
---
 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 5cdabf8ae9da3..279926e1d4c1c 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

Re: [PATCH 3/5] ASoC: SDCA: Fix cleanup inversion in class driver

[Mark Brown]

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

On Wed, Apr 08, 2026 at 10:38:33AM +0100, Charles Keepax wrote:

> 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.

> +static void class_remove_functions(void *data)
> +{
> +	struct sdca_class_drv *drv = data;
> +
> +	cancel_work_sync(&drv->boot_work);
> +
> +	sdca_dev_unregister_functions(drv->sdw);
> +}

> @@ -157,6 +166,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;
> +

This means we register the cleanup of boot_work from boot_work which
seems a bit fun and...

> -static void class_dev_remove(void *data)
> -{
> -	struct sdca_class_drv *drv = data;
> -
> -	cancel_work_sync(&drv->boot_work);

...what now cancels boot_work if we somehow manage to remove before
boot_work has completed (possibly even before it's scheduled)?

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

Re: [PATCH 3/5] ASoC: SDCA: Fix cleanup inversion in class driver

[Charles Keepax]

On Wed, Apr 08, 2026 at 02:43:59PM +0100, Mark Brown wrote:
> On Wed, Apr 08, 2026 at 10:38:33AM +0100, Charles Keepax wrote:
> 
> > 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.
> 
> > +static void class_remove_functions(void *data)
> > +{
> > +	struct sdca_class_drv *drv = data;
> > +
> > +	cancel_work_sync(&drv->boot_work);
> > +
> > +	sdca_dev_unregister_functions(drv->sdw);
> > +}
> 
> > @@ -157,6 +166,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;
> > +
> 
> This means we register the cleanup of boot_work from boot_work which
> seems a bit fun and...
> 
> > -static void class_dev_remove(void *data)
> > -{
> > -	struct sdca_class_drv *drv = data;
> > -
> > -	cancel_work_sync(&drv->boot_work);
> 
> ...what now cancels boot_work if we somehow manage to remove before
> boot_work has completed (possibly even before it's scheduled)?

Hm... yeah I think you are right there is still an issue here.

I think the boot work sync should stay in the original remove
callback. The sequence of concern was:

1) driver probes
2) boot work runs
3) boot work is scheduled
4) driver removed
5) boot work completes

I think this current patch addresses calling
sdca_dev_unregister_functions() but we don't really want any of
boot work running once the driver starts being removed.

I will respin a v2.

Thanks,
Charles