[PATCH v2 0/4] SDCA System Suspend Support

[PATCH v2 0/4] SDCA System Suspend Support

[Charles Keepax]

Add support for system suspend into the class driver, now split
out into a separate patch chain.

Where we got to on the previous discussion, was we don't currently
have any parts requiring download on runtime resume, doing so
will add noticeable delay to the runtime resume, and we are not
blocking someone from adding support for firmware download on
runtime resume in the future. Also as runtime resume is really
a kernel concept and power rails are primarily controlled by
ACPI it is quite unlikely anyone will actually power down the
part on a runtime suspend anyway. So this version of the chain
still only downloads firmware on probe and system resume.

Thanks,
Charles

Changes since v1:
 - Update SDCA IRQ enable/disable API to be more clear.

Charles Keepax (4):
  ASoC: SDCA: Add SDCA IRQ enable/disable helpers
  ASoC: SDCA: Add basic system suspend support
  ASoC: SDCA: Device boot into the system suspend process
  ASoC: SDCA: Add lock to serialise the Function initialisation

 include/sound/sdca_interrupts.h      |   7 ++
 sound/soc/sdca/sdca_class.c          |  34 ++++++++
 sound/soc/sdca/sdca_class.h          |   2 +
 sound/soc/sdca/sdca_class_function.c | 117 ++++++++++++++++++++++-----
 sound/soc/sdca/sdca_interrupts.c     |  89 ++++++++++++++++++--
 5 files changed, 223 insertions(+), 26 deletions(-)

-- 
2.47.3

[PATCH v2 1/4] ASoC: SDCA: Add SDCA IRQ enable/disable helpers

[Charles Keepax]

Add helpers to enable and disable the SDCA IRQs by Function. These are
useful to sequence the powering down and up around system suspend.

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

Changes since v1:
 - Rather than flagging the early IRQs with a boolean flag provide a
   helper with a different function name. This makes it clearer what the
   calling code is doing.

 include/sound/sdca_interrupts.h  |  7 ++++
 sound/soc/sdca/sdca_interrupts.c | 72 ++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index 8f13417d129ab..9bcb5d8fd592b 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -84,4 +84,11 @@ int sdca_irq_populate(struct sdca_function_data *function,
 struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
 					      struct regmap *regmap, int irq);
 
+void sdca_irq_enable_early(struct sdca_function_data *function,
+			   struct sdca_interrupt_info *info);
+void sdca_irq_enable(struct sdca_function_data *function,
+		     struct sdca_interrupt_info *info);
+void sdca_irq_disable(struct sdca_function_data *function,
+		      struct sdca_interrupt_info *info);
+
 #endif
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 8f6a2adfb6fbe..928355f5b1003 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -610,3 +610,75 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *sdev,
 	return info;
 }
 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];
+
+		if (!interrupt || interrupt->function != function)
+			continue;
+
+		switch (SDCA_CTL_TYPE(interrupt->entity->type,
+				      interrupt->control->sel)) {
+		case SDCA_CTL_TYPE_S(XU, FDL_CURRENTOWNER):
+			if (early)
+				enable_irq(interrupt->irq);
+			break;
+		default:
+			if (!early)
+				enable_irq(interrupt->irq);
+			break;
+		}
+	}
+}
+
+/**
+ * sdca_irq_enable_early - Re-enable early SDCA IRQs for a given function
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ */
+void sdca_irq_enable_early(struct sdca_function_data *function,
+			   struct sdca_interrupt_info *info)
+{
+	irq_enable_flags(function, info, true);
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_enable_early, "SND_SOC_SDCA");
+
+/**
+ * sdca_irq_enable_early - Re-enable SDCA IRQs for a given function
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ */
+void sdca_irq_enable(struct sdca_function_data *function,
+		     struct sdca_interrupt_info *info)
+{
+	irq_enable_flags(function, info, false);
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_enable, "SND_SOC_SDCA");
+
+/**
+ * sdca_irq_disable - Disable SDCA IRQs for a given function
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ */
+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];
+
+		if (!interrupt || interrupt->function != function)
+			continue;
+
+		disable_irq(interrupt->irq);
+	}
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_disable, "SND_SOC_SDCA");
-- 
2.47.3

[PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Charles Keepax]

Add basic system suspend support. Disable the IRQs and force runtime
suspend, during system suspend, because the device will likely fully
power down during suspend.

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

Changes since v1:
 - Use new enable/disable API

 sound/soc/sdca/sdca_class.c          | 33 ++++++++++++++++++++++
 sound/soc/sdca/sdca_class_function.c | 41 ++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 349d32933ba85..438695291257e 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -238,6 +238,38 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
 	return 0;
 }
 
+static int class_suspend(struct device *dev)
+{
+	struct sdca_class_drv *drv = dev_get_drvdata(dev);
+	int ret;
+
+	disable_irq(drv->sdw->irq);
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret) {
+		dev_err(dev, "Failed to force suspend: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int class_resume(struct device *dev)
+{
+	struct sdca_class_drv *drv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret) {
+		dev_err(dev, "Failed to force resume: %d\n", ret);
+		return ret;
+	}
+
+	enable_irq(drv->sdw->irq);
+
+	return 0;
+}
+
 static int class_runtime_suspend(struct device *dev)
 {
 	struct sdca_class_drv *drv = dev_get_drvdata(dev);
@@ -278,6 +310,7 @@ static int class_runtime_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops class_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(class_suspend, class_resume)
 	RUNTIME_PM_OPS(class_runtime_suspend, class_runtime_resume, NULL)
 };
 
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 0028482a1e752..8f4772a5ca4e5 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -32,6 +32,7 @@ struct class_function_drv {
 	struct sdca_class_drv *core;
 
 	struct sdca_function_data *function;
+	bool suspended;
 };
 
 static void class_function_regmap_lock(void *data)
@@ -404,6 +405,13 @@ static int class_function_runtime_resume(struct device *dev)
 	regcache_mark_dirty(drv->regmap);
 	regcache_cache_only(drv->regmap, false);
 
+	if (drv->suspended) {
+		sdca_irq_enable_early(drv->function, drv->core->irq_info);
+		sdca_irq_enable(drv->function, drv->core->irq_info);
+
+		drv->suspended = false;
+	}
+
 	ret = regcache_sync(drv->regmap);
 	if (ret) {
 		dev_err(drv->dev, "failed to restore register cache: %d\n", ret);
@@ -418,7 +426,40 @@ static int class_function_runtime_resume(struct device *dev)
 	return ret;
 }
 
+static int class_function_suspend(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+	int ret;
+
+	drv->suspended = true;
+
+	sdca_irq_disable(drv->function, drv->core->irq_info);
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret) {
+		dev_err(dev, "Failed to force suspend: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int class_function_resume(struct device *dev)
+{
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret) {
+		dev_err(dev, "Failed to force resume: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static const struct dev_pm_ops class_function_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(class_function_suspend, class_function_resume)
 	RUNTIME_PM_OPS(class_function_runtime_suspend,
 		       class_function_runtime_resume, NULL)
 };
-- 
2.47.3

[PATCH v2 3/4] ASoC: SDCA: Device boot into the system suspend process

[Charles Keepax]

When system suspending the device may be powered off, this means all
state will be lost and the firmware may need to be re-downloaded. Add
the necessary calls to bring the device back up. This also requires that
that the FDL (firmware download) IRQ handler is modified to allow it to
run before runtime PM has been fully restored.

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

No changes since v1.

 sound/soc/sdca/sdca_class_function.c | 71 ++++++++++++++++++++--------
 sound/soc/sdca/sdca_interrupts.c     | 17 +++++--
 2 files changed, 62 insertions(+), 26 deletions(-)

diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 8f4772a5ca4e5..5a818d958543f 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -201,21 +201,12 @@ static const struct snd_soc_component_driver class_function_component_drv = {
 	.endianness		= 1,
 };
 
-static int class_function_boot(struct class_function_drv *drv)
+static int class_function_init_device(struct class_function_drv *drv,
+				      unsigned int status)
 {
-	unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
-					SDCA_ENTITY_TYPE_ENTITY_0,
-					SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
-	unsigned int val;
 	int ret;
 
-	ret = regmap_read(drv->regmap, reg, &val);
-	if (ret < 0) {
-		dev_err(drv->dev, "failed to read function status: %d\n", ret);
-		return ret;
-	}
-
-	if (!(val & SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET)) {
+	if (!(status & SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET)) {
 		dev_dbg(drv->dev, "reset function device\n");
 
 		ret = sdca_reset_function(drv->dev, drv->function, drv->regmap);
@@ -223,24 +214,36 @@ static int class_function_boot(struct class_function_drv *drv)
 			return ret;
 	}
 
-	if (val & SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION) {
+	if (status & SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION) {
 		dev_dbg(drv->dev, "write initialisation\n");
 
 		ret = sdca_regmap_write_init(drv->dev, drv->core->dev_regmap,
 					     drv->function);
 		if (ret)
 			return ret;
+	}
 
-		ret = regmap_write(drv->regmap, reg,
-				   SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION);
-		if (ret < 0) {
-			dev_err(drv->dev,
-				"failed to clear function init status: %d\n",
-				ret);
-			return ret;
-		}
+	return 0;
+}
+
+static int class_function_boot(struct class_function_drv *drv)
+{
+	unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
+					SDCA_ENTITY_TYPE_ENTITY_0,
+					SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(drv->regmap, reg, &val);
+	if (ret < 0) {
+		dev_err(drv->dev, "failed to read function status: %d\n", ret);
+		return ret;
 	}
 
+	ret = class_function_init_device(drv, val);
+	if (ret)
+		return ret;
+
 	/* Start FDL process */
 	ret = sdca_irq_populate_early(drv->dev, drv->regmap, drv->function,
 				      drv->core->irq_info);
@@ -406,9 +409,35 @@ static int class_function_runtime_resume(struct device *dev)
 	regcache_cache_only(drv->regmap, false);
 
 	if (drv->suspended) {
+		unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
+						SDCA_ENTITY_TYPE_ENTITY_0,
+						SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
+		unsigned int val;
+
+		ret = regmap_read(drv->regmap, reg, &val);
+		if (ret < 0) {
+			dev_err(drv->dev, "failed to read function status: %d\n", ret);
+			goto err;
+		}
+
+		ret = class_function_init_device(drv, val);
+		if (ret)
+			goto err;
+
 		sdca_irq_enable_early(drv->function, drv->core->irq_info);
+
+		ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
+		if (ret)
+			goto err;
+
 		sdca_irq_enable(drv->function, drv->core->irq_info);
 
+		ret = regmap_write(drv->regmap, reg, 0xFF);
+		if (ret < 0) {
+			dev_err(drv->dev, "failed to clear function status: %d\n", ret);
+			goto err;
+		}
+
 		drv->suspended = false;
 	}
 
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 928355f5b1003..9666b1e6cf1f8 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -278,10 +278,16 @@ static irqreturn_t fdl_owner_handler(int irq, void *data)
 	irqreturn_t irqret = IRQ_NONE;
 	int ret;
 
-	ret = pm_runtime_get_sync(dev);
-	if (ret < 0) {
-		dev_err(dev, "failed to resume for fdl: %d\n", ret);
-		goto error;
+	/*
+	 * FDL has to run from the system resume handler, at which point
+	 * pm_runtime isn't yet active.
+	 */
+	if (pm_runtime_active(dev)) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to resume for fdl: %d\n", ret);
+			goto error;
+		}
 	}
 
 	ret = sdca_fdl_process(interrupt);
@@ -290,7 +296,8 @@ static irqreturn_t fdl_owner_handler(int irq, void *data)
 
 	irqret = IRQ_HANDLED;
 error:
-	pm_runtime_put(dev);
+	if (pm_runtime_active(dev))
+		pm_runtime_put(dev);
 	return irqret;
 }
 
-- 
2.47.3

[PATCH v2 4/4] ASoC: SDCA: Add lock to serialise the Function initialisation

[Charles Keepax]

To avoid issues on some devices serialise the boot of each SDCA Function
from the others.

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

No changes since v1.

 sound/soc/sdca/sdca_class.c          | 1 +
 sound/soc/sdca/sdca_class.h          | 2 ++
 sound/soc/sdca/sdca_class_function.c | 5 +++++
 3 files changed, 8 insertions(+)

diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 438695291257e..1b83aad1d9d3b 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -205,6 +205,7 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
 	drv->dev = dev;
 	drv->sdw = sdw;
 	mutex_init(&drv->regmap_lock);
+	mutex_init(&drv->init_lock);
 
 	dev_set_drvdata(drv->dev, drv);
 
diff --git a/sound/soc/sdca/sdca_class.h b/sound/soc/sdca/sdca_class.h
index bb4c9dd124296..6f24ea2bbd381 100644
--- a/sound/soc/sdca/sdca_class.h
+++ b/sound/soc/sdca/sdca_class.h
@@ -28,6 +28,8 @@ struct sdca_class_drv {
 	struct sdca_interrupt_info *irq_info;
 
 	struct mutex regmap_lock;
+	/* Serialise function initialisations */
+	struct mutex init_lock;
 	struct work_struct boot_work;
 	struct completion device_attach;
 
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 5a818d958543f..b4d2e40efe174 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/auxiliary_bus.h>
+#include <linux/cleanup.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/pm.h>
@@ -234,6 +235,8 @@ static int class_function_boot(struct class_function_drv *drv)
 	unsigned int val;
 	int ret;
 
+	guard(mutex)(&drv->core->init_lock);
+
 	ret = regmap_read(drv->regmap, reg, &val);
 	if (ret < 0) {
 		dev_err(drv->dev, "failed to read function status: %d\n", ret);
@@ -405,6 +408,8 @@ static int class_function_runtime_resume(struct device *dev)
 	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
 	int ret;
 
+	guard(mutex)(&drv->core->init_lock);
+
 	regcache_mark_dirty(drv->regmap);
 	regcache_cache_only(drv->regmap, false);
 
-- 
2.47.3

Re: [PATCH v2 1/4] ASoC: SDCA: Add SDCA IRQ enable/disable helpers

[Pierre-Louis Bossart]

only a couple of nit-picks below:

> +/**
> + * sdca_irq_enable_early - Re-enable early SDCA IRQs for a given function
> + * @function: Pointer to the SDCA Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + */
> +void sdca_irq_enable_early(struct sdca_function_data *function,
> +			   struct sdca_interrupt_info *info)
> +{
> +	irq_enable_flags(function, info, true);
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_enable_early, "SND_SOC_SDCA");
> +
> +/**
> + * sdca_irq_enable_early - Re-enable SDCA IRQs for a given function

copy-paste from above making this kernel-doc description incorrect.

> + * @function: Pointer to the SDCA Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + */
> +void sdca_irq_enable(struct sdca_function_data *function,
> +		     struct sdca_interrupt_info *info)
> +{
> +	irq_enable_flags(function, info, false);
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_enable, "SND_SOC_SDCA");
> +
> +/**
> + * sdca_irq_disable - Disable SDCA IRQs for a given function
> + * @function: Pointer to the SDCA Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + */
> +void sdca_irq_disable(struct sdca_function_data *function,
> +		      struct sdca_interrupt_info *info)

maybe explain why we have an _early version for enable but not a _late for disable?

> +{
> +	struct sdca_interrupt *interrupt;
> +	int i;
> +
> +	for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
> +		interrupt = &info->irqs[i];
> +
> +		if (!interrupt || interrupt->function != function)
> +			continue;
> +
> +		disable_irq(interrupt->irq);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_disable, "SND_SOC_SDCA");

Re: [PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Pierre-Louis Bossart]

> +static int class_suspend(struct device *dev)
> +{
> +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	disable_irq(drv->sdw->irq);
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to force suspend: %d\n", ret);

suggestion: since this is going to be fairly generic code used by multiple devices, it'd be useful to add a device identifier or string to the error log, no?

This can be done later in a follow-up patch if agreed.


> +	if (drv->suspended) {
> +		sdca_irq_enable_early(drv->function, drv->core->irq_info);
> +		sdca_irq_enable(drv->function, drv->core->irq_info);

maybe add a comment on why the two calls are done sequentially. You did mention it somewhere, it'd be good to capture the design.

looks good otherwise.

Re: [PATCH v2 1/4] ASoC: SDCA: Add SDCA IRQ enable/disable helpers

[Charles Keepax]

On Tue, Jan 06, 2026 at 06:17:10PM +0100, Pierre-Louis Bossart wrote:
> only a couple of nit-picks below:
> 
> > +/**
> > + * sdca_irq_enable_early - Re-enable early SDCA IRQs for a given function
> > + * @function: Pointer to the SDCA Function.
> > + * @info: Pointer to the SDCA interrupt info for this device.
> > + */
> > +void sdca_irq_enable_early(struct sdca_function_data *function,
> > +			   struct sdca_interrupt_info *info)
> > +{
> > +	irq_enable_flags(function, info, true);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(sdca_irq_enable_early, "SND_SOC_SDCA");
> > +
> > +/**
> > + * sdca_irq_enable_early - Re-enable SDCA IRQs for a given function
> 
> copy-paste from above making this kernel-doc description incorrect.

Thanks, good spot will get that fixed up.

> > + * @function: Pointer to the SDCA Function.
> > + * @info: Pointer to the SDCA interrupt info for this device.
> > + */
> > +void sdca_irq_enable(struct sdca_function_data *function,
> > +		     struct sdca_interrupt_info *info)
> > +{
> > +	irq_enable_flags(function, info, false);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(sdca_irq_enable, "SND_SOC_SDCA");
> > +
> > +/**
> > + * sdca_irq_disable - Disable SDCA IRQs for a given function
> > + * @function: Pointer to the SDCA Function.
> > + * @info: Pointer to the SDCA interrupt info for this device.
> > + */
> > +void sdca_irq_disable(struct sdca_function_data *function,
> > +		      struct sdca_interrupt_info *info)
> 
> maybe explain why we have an _early version for enable but not a _late for disable?
> 

Yeah I can add some text to that affect.

Thanks,
Charles

Re: [PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Charles Keepax]

On Tue, Jan 06, 2026 at 06:23:17PM +0100, Pierre-Louis Bossart wrote:
> 
> > +static int class_suspend(struct device *dev)
> > +{
> > +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	disable_irq(drv->sdw->irq);
> > +
> > +	ret = pm_runtime_force_suspend(dev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to force suspend: %d\n", ret);
> 
> suggestion: since this is going to be fairly generic code
> used by multiple devices, it'd be useful to add a device
> identifier or string to the error log, no?

Doesn't the dev_err already do that, it prints the device name in
the log?

> > +	if (drv->suspended) {
> > +		sdca_irq_enable_early(drv->function, drv->core->irq_info);
> > +		sdca_irq_enable(drv->function, drv->core->irq_info);
> 
> maybe add a comment on why the two calls are done
> sequentially. You did mention it somewhere, it'd be good to
> capture the design.

Yeah can do, its a little bit of one of these intermediate step
things. Becomes come clear in the next patch.

Thanks,
Charles

Re: [PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Pierre-Louis Bossart]

On 1/7/26 11:04, Charles Keepax wrote:
> On Tue, Jan 06, 2026 at 06:23:17PM +0100, Pierre-Louis Bossart wrote:
>>
>>> +static int class_suspend(struct device *dev)
>>> +{
>>> +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	disable_irq(drv->sdw->irq);
>>> +
>>> +	ret = pm_runtime_force_suspend(dev);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to force suspend: %d\n", ret);
>>
>> suggestion: since this is going to be fairly generic code
>> used by multiple devices, it'd be useful to add a device
>> identifier or string to the error log, no?
> 
> Doesn't the dev_err already do that, it prints the device name in
> the log?

Yes but the device name could be confusing, not everyone can figure out what "sdw:0:0:025d:0711:01:" means

>>> +	if (drv->suspended) {
>>> +		sdca_irq_enable_early(drv->function, drv->core->irq_info);
>>> +		sdca_irq_enable(drv->function, drv->core->irq_info);
>>
>> maybe add a comment on why the two calls are done
>> sequentially. You did mention it somewhere, it'd be good to
>> capture the design.
> 
> Yeah can do, its a little bit of one of these intermediate step
> things. Becomes come clear in the next patch.

ok

Re: [PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Charles Keepax]

On Thu, Jan 08, 2026 at 11:18:56AM +0100, Pierre-Louis Bossart wrote:
> On 1/7/26 11:04, Charles Keepax wrote:
> > On Tue, Jan 06, 2026 at 06:23:17PM +0100, Pierre-Louis Bossart wrote:
> >>
> >>> +static int class_suspend(struct device *dev)
> >>> +{
> >>> +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> >>> +	int ret;
> >>> +
> >>> +	disable_irq(drv->sdw->irq);
> >>> +
> >>> +	ret = pm_runtime_force_suspend(dev);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "Failed to force suspend: %d\n", ret);
> >>
> >> suggestion: since this is going to be fairly generic code
> >> used by multiple devices, it'd be useful to add a device
> >> identifier or string to the error log, no?
> > 
> > Doesn't the dev_err already do that, it prints the device name in
> > the log?
> 
> Yes but the device name could be confusing, not everyone
> can figure out what "sdw:0:0:025d:0711:01:" means

Personally I think if you are interfacing with these things you
need to be able to interpret that anyway, since it will affect
things like finding things in debugfs or sysfs as well.

However, if we do want to update the error messages to include
some additional identifiers that should really be done for the
whole class driver. So it would make more sense to do as a
separate patch chain, rather than as part of this suspend chain.

Thanks,
Charles

Re: [PATCH v2 2/4] ASoC: SDCA: Add basic system suspend support

[Pierre-Louis Bossart]

>>>>> +	ret = pm_runtime_force_suspend(dev);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "Failed to force suspend: %d\n", ret);
>>>>
>>>> suggestion: since this is going to be fairly generic code
>>>> used by multiple devices, it'd be useful to add a device
>>>> identifier or string to the error log, no?
>>>
>>> Doesn't the dev_err already do that, it prints the device name in
>>> the log?
>>
>> Yes but the device name could be confusing, not everyone
>> can figure out what "sdw:0:0:025d:0711:01:" means
> 
> Personally I think if you are interfacing with these things you
> need to be able to interpret that anyway, since it will affect
> things like finding things in debugfs or sysfs as well.
> 
> However, if we do want to update the error messages to include
> some additional identifiers that should really be done for the
> whole class driver. So it would make more sense to do as a
> separate patch chain, rather than as part of this suspend chain.

Yes agreed, my suggestion was for future work.
The main thing was that for a class driver it's a bit odd that all the messages refer to the manufacturer and device ID. The two-level device structure will become even more confusing...