[PATCH 0/4] Add SDCA class driver

[PATCH 0/4] Add SDCA class driver

[Charles Keepax]

This series adds an initial SDCA class driver, this consists of a
primary driver attached to the SoundWire device, and auxiliary drivers
representing each of the functions of the SDCA device. These drivers all
use the APIs added over the past series's to provide the class
functionality, as such these final drivers themselves are quite thin.

Note this series depends on the UMP/FDL series that is still in review,
but it has been sent a little early to get a head start on reviews.

https://lore.kernel.org/linux-sound/20250925105341.194178-1-ckeepax@opensource.cirrus.com/T/#t

Thanks,
Charles

Charles Keepax (3):
  ASoC: SDCA: Add helper to write initialization writes
  ASoC: SDCA: Add basic SDCA class driver
  ASoC: SDCA: Add basic SDCA function driver

Pierre-Louis Bossart (1):
  ASoC: SDCA: add function devices

 include/linux/soundwire/sdw_registers.h |   2 +
 include/sound/sdca.h                    |  13 +
 include/sound/sdca_regmap.h             |   2 +
 sound/soc/sdca/Kconfig                  |  18 +
 sound/soc/sdca/Makefile                 |  10 +-
 sound/soc/sdca/sdca_class.c             | 304 ++++++++++++++++
 sound/soc/sdca/sdca_class.h             |  37 ++
 sound/soc/sdca/sdca_class_function.c    | 459 ++++++++++++++++++++++++
 sound/soc/sdca/sdca_function_device.c   | 117 ++++++
 sound/soc/sdca/sdca_function_device.h   |  15 +
 sound/soc/sdca/sdca_regmap.c            |  16 +
 11 files changed, 991 insertions(+), 2 deletions(-)
 create mode 100644 sound/soc/sdca/sdca_class.c
 create mode 100644 sound/soc/sdca/sdca_class.h
 create mode 100644 sound/soc/sdca/sdca_class_function.c
 create mode 100644 sound/soc/sdca/sdca_function_device.c
 create mode 100644 sound/soc/sdca/sdca_function_device.h

-- 
2.47.3

[PATCH 1/4] ASoC: SDCA: Add helper to write initialization writes

[Charles Keepax]

Add a helper function to write out the SDCA blind initialization writes.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_regmap.h  |  2 ++
 sound/soc/sdca/sdca_regmap.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/sound/sdca_regmap.h b/include/sound/sdca_regmap.h
index b2e3c2ad2bb88..792540a530fc4 100644
--- a/include/sound/sdca_regmap.h
+++ b/include/sound/sdca_regmap.h
@@ -27,5 +27,7 @@ int sdca_regmap_populate_constants(struct device *dev, struct sdca_function_data
 
 int sdca_regmap_write_defaults(struct device *dev, struct regmap *regmap,
 			       struct sdca_function_data *function);
+int sdca_regmap_write_init(struct device *dev, struct regmap *regmap,
+			   struct sdca_function_data *function);
 
 #endif // __SDCA_REGMAP_H__
diff --git a/sound/soc/sdca/sdca_regmap.c b/sound/soc/sdca/sdca_regmap.c
index 8fa138fca00ff..057862b6c9af5 100644
--- a/sound/soc/sdca/sdca_regmap.c
+++ b/sound/soc/sdca/sdca_regmap.c
@@ -326,3 +326,19 @@ int sdca_regmap_write_defaults(struct device *dev, struct regmap *regmap,
 	return 0;
 }
 EXPORT_SYMBOL_NS(sdca_regmap_write_defaults, "SND_SOC_SDCA");
+
+int sdca_regmap_write_init(struct device *dev, struct regmap *regmap,
+			   struct sdca_function_data *function)
+{
+	struct sdca_init_write *init = function->init_table;
+	int ret, i;
+
+	for (i = 0; i < function->num_init_table; i++) {
+		ret = regmap_write(regmap, init[i].addr, init[i].val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_regmap_write_init, "SND_SOC_SDCA");
-- 
2.47.3

Re: [PATCH 1/4] ASoC: SDCA: Add helper to write initialization writes

[Vinod Koul]

On 25-09-25, 14:33, Charles Keepax wrote:
> Add a helper function to write out the SDCA blind initialization writes.

Acked-by: Vinod Koul <vkoul@kernel.org>


-- 
~Vinod

[PATCH 2/4] ASoC: SDCA: add function devices

[Charles Keepax]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use the auxiliary bus to register/unregister subdevices for each
function. Each function will be handled with a separate driver,
matched using a name.

If a vendor wants to override a specific function driver, they could
use a custom name to match with a custom function driver.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca.h                  |  13 +++
 sound/soc/sdca/Kconfig                |   1 +
 sound/soc/sdca/Makefile               |   4 +-
 sound/soc/sdca/sdca_function_device.c | 117 ++++++++++++++++++++++++++
 sound/soc/sdca/sdca_function_device.h |  15 ++++
 5 files changed, 148 insertions(+), 2 deletions(-)
 create mode 100644 sound/soc/sdca/sdca_function_device.c
 create mode 100644 sound/soc/sdca/sdca_function_device.h

diff --git a/include/sound/sdca.h b/include/sound/sdca.h
index d38cdbfeb35f5..ae369287797d4 100644
--- a/include/sound/sdca.h
+++ b/include/sound/sdca.h
@@ -14,18 +14,21 @@
 
 struct acpi_table_swft;
 struct sdw_slave;
+struct sdca_dev;
 
 #define SDCA_MAX_FUNCTION_COUNT 8
 
 /**
  * struct sdca_function_desc - short descriptor for an SDCA Function
  * @node: firmware node for the Function.
+ * @func_dev: pointer to SDCA function device.
  * @name: Human-readable string.
  * @type: Function topology type.
  * @adr: ACPI address (used for SDCA register access).
  */
 struct sdca_function_desc {
 	struct fwnode_handle *node;
+	struct sdca_dev *func_dev;
 	const char *name;
 	u32 type;
 	u8 adr;
@@ -58,6 +61,8 @@ void sdca_lookup_functions(struct sdw_slave *slave);
 void sdca_lookup_swft(struct sdw_slave *slave);
 void sdca_lookup_interface_revision(struct sdw_slave *slave);
 bool sdca_device_quirk_match(struct sdw_slave *slave, enum sdca_quirk quirk);
+int sdca_dev_register_functions(struct sdw_slave *slave);
+void sdca_dev_unregister_functions(struct sdw_slave *slave);
 
 #else
 
@@ -68,6 +73,14 @@ static inline bool sdca_device_quirk_match(struct sdw_slave *slave, enum sdca_qu
 {
 	return false;
 }
+
+static inline int sdca_dev_register_functions(struct sdw_slave *slave)
+{
+	return 0;
+}
+
+static inline void sdca_dev_unregister_functions(struct sdw_slave *slave) {}
+
 #endif
 
 #endif
diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
index a73920d07073e..e7f36d668f159 100644
--- a/sound/soc/sdca/Kconfig
+++ b/sound/soc/sdca/Kconfig
@@ -4,6 +4,7 @@ menu "SoundWire (SDCA)"
 config SND_SOC_SDCA
 	tristate
 	depends on ACPI
+	select AUXILIARY_BUS
 	help
 	  This option enables support for the MIPI SoundWire Device
 	  Class for Audio (SDCA).
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index be911c399bbde..babe3fa2bb3ff 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-snd-soc-sdca-y := sdca_functions.o sdca_device.o sdca_regmap.o sdca_asoc.o \
-		  sdca_ump.o
+snd-soc-sdca-y := sdca_functions.o sdca_device.o sdca_function_device.o \
+		  sdca_regmap.o sdca_asoc.o sdca_ump.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_HID) += sdca_hid.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_IRQ) += sdca_interrupts.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_FDL) += sdca_fdl.o
diff --git a/sound/soc/sdca/sdca_function_device.c b/sound/soc/sdca/sdca_function_device.c
new file mode 100644
index 0000000000000..91c49d7389db0
--- /dev/null
+++ b/sound/soc/sdca/sdca_function_device.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+// Copyright(c) 2024 Intel Corporation.
+
+/*
+ * SDCA Function Device management
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/soundwire/sdw.h>
+#include <sound/sdca.h>
+#include <sound/sdca_function.h>
+#include "sdca_function_device.h"
+
+/*
+ * A SoundWire device can have multiple SDCA functions identified by
+ * their type and ADR. there can be multiple SoundWire devices per
+ * link, or multiple devices spread across multiple links. An IDA is
+ * required to identify each instance.
+ */
+static DEFINE_IDA(sdca_function_ida);
+
+static void sdca_dev_release(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct sdca_dev *sdev = auxiliary_dev_to_sdca_dev(auxdev);
+
+	ida_free(&sdca_function_ida, auxdev->id);
+	kfree(sdev);
+}
+
+/* alloc, init and add link devices */
+static struct sdca_dev *sdca_dev_register(struct device *parent,
+					  struct sdca_function_desc *function_desc)
+{
+	struct sdca_dev *sdev;
+	struct auxiliary_device *auxdev;
+	int ret;
+	int rc;
+
+	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+	if (!sdev)
+		return ERR_PTR(-ENOMEM);
+
+	auxdev = &sdev->auxdev;
+	auxdev->name = function_desc->name;
+	auxdev->dev.parent = parent;
+	auxdev->dev.fwnode = function_desc->node;
+	auxdev->dev.release = sdca_dev_release;
+
+	sdev->function.desc = function_desc;
+
+	rc = ida_alloc(&sdca_function_ida, GFP_KERNEL);
+	if (rc < 0) {
+		kfree(sdev);
+		return ERR_PTR(rc);
+	}
+	auxdev->id = rc;
+
+	/* now follow the two-step init/add sequence */
+	ret = auxiliary_device_init(auxdev);
+	if (ret < 0) {
+		dev_err(parent, "failed to initialize SDCA function dev %s\n",
+			function_desc->name);
+		ida_free(&sdca_function_ida, auxdev->id);
+		kfree(sdev);
+		return ERR_PTR(ret);
+	}
+
+	ret = auxiliary_device_add(auxdev);
+	if (ret < 0) {
+		dev_err(parent, "failed to add SDCA function dev %s\n",
+			sdev->auxdev.name);
+		/* sdev will be freed with the put_device() and .release sequence */
+		auxiliary_device_uninit(&sdev->auxdev);
+		return ERR_PTR(ret);
+	}
+
+	return sdev;
+}
+
+static void sdca_dev_unregister(struct sdca_dev *sdev)
+{
+	auxiliary_device_delete(&sdev->auxdev);
+	auxiliary_device_uninit(&sdev->auxdev);
+}
+
+int sdca_dev_register_functions(struct sdw_slave *slave)
+{
+	struct sdca_device_data *sdca_data = &slave->sdca_data;
+	int i;
+
+	for (i = 0; i < sdca_data->num_functions; i++) {
+		struct sdca_dev *func_dev;
+
+		func_dev = sdca_dev_register(&slave->dev,
+					     &sdca_data->function[i]);
+		if (!func_dev)
+			return -ENODEV;
+
+		sdca_data->function[i].func_dev = func_dev;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_dev_register_functions, "SND_SOC_SDCA");
+
+void sdca_dev_unregister_functions(struct sdw_slave *slave)
+{
+	struct sdca_device_data *sdca_data = &slave->sdca_data;
+	int i;
+
+	for (i = 0; i < sdca_data->num_functions; i++)
+		sdca_dev_unregister(sdca_data->function[i].func_dev);
+}
+EXPORT_SYMBOL_NS(sdca_dev_unregister_functions, "SND_SOC_SDCA");
diff --git a/sound/soc/sdca/sdca_function_device.h b/sound/soc/sdca/sdca_function_device.h
new file mode 100644
index 0000000000000..5adf7551d3a44
--- /dev/null
+++ b/sound/soc/sdca/sdca_function_device.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/* Copyright(c) 2024 Intel Corporation. */
+
+#ifndef __SDCA_FUNCTION_DEVICE_H
+#define __SDCA_FUNCTION_DEVICE_H
+
+struct sdca_dev {
+	struct auxiliary_device auxdev;
+	struct sdca_function_data function;
+};
+
+#define auxiliary_dev_to_sdca_dev(auxiliary_dev)		\
+	container_of(auxiliary_dev, struct sdca_dev, auxdev)
+
+#endif
-- 
2.47.3

[PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

Add basic driver to support core of the class driver.

Co-developed-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/linux/soundwire/sdw_registers.h |   2 +
 sound/soc/sdca/Kconfig                  |  10 +
 sound/soc/sdca/Makefile                 |   4 +
 sound/soc/sdca/sdca_class.c             | 304 ++++++++++++++++++++++++
 sound/soc/sdca/sdca_class.h             |  37 +++
 5 files changed, 357 insertions(+)
 create mode 100644 sound/soc/sdca/sdca_class.c
 create mode 100644 sound/soc/sdca/sdca_class.h

diff --git a/include/linux/soundwire/sdw_registers.h b/include/linux/soundwire/sdw_registers.h
index 0a5939285583b..cae8a0a5a9b04 100644
--- a/include/linux/soundwire/sdw_registers.h
+++ b/include/linux/soundwire/sdw_registers.h
@@ -355,4 +355,6 @@
 /* Check the reserved and fixed bits in address */
 #define SDW_SDCA_VALID_CTL(reg) (((reg) & (GENMASK(31, 25) | BIT(18) | BIT(13))) == BIT(30))
 
+#define SDW_SDCA_MAX_REGISTER			0x47FFFFFF
+
 #endif /* __SDW_REGISTERS_H */
diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
index e7f36d668f159..56e3ff8448762 100644
--- a/sound/soc/sdca/Kconfig
+++ b/sound/soc/sdca/Kconfig
@@ -37,4 +37,14 @@ config SND_SOC_SDCA_FDL
 config SND_SOC_SDCA_OPTIONAL
 	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
 
+config SND_SOC_SDCA_CLASS
+	tristate "SDCA Class Driver"
+	select SND_SOC_SDCA
+	select SND_SOC_SDCA_FDL
+	select SND_SOC_SDCA_HID
+	select SND_SOC_SDCA_IRQ
+	help
+	  This option enables support for the SDCA Class driver which should
+	  support any class compliant SDCA part.
+
 endmenu
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index babe3fa2bb3ff..95db4cef34833 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -6,4 +6,8 @@ snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_HID) += sdca_hid.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_IRQ) += sdca_interrupts.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_FDL) += sdca_fdl.o
 
+snd-soc-sdca-class-y := sdca_class.o
+
 obj-$(CONFIG_SND_SOC_SDCA) += snd-soc-sdca.o
+
+obj-$(CONFIG_SND_SOC_SDCA_CLASS) += snd-soc-sdca-class.o
diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
new file mode 100644
index 0000000000000..16e26574bc254
--- /dev/null
+++ b/sound/soc/sdca/sdca_class.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Cirrus Logic, Inc. and
+//                    Cirrus Logic International Semiconductor Ltd.
+
+/*
+ * The MIPI SDCA specification is available for public downloads at
+ * https://www.mipi.org/mipi-sdca-v1-0-download
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <linux/soundwire/sdw_type.h>
+#include <sound/sdca.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/sdca_regmap.h>
+#include "sdca_class.h"
+
+#define CLASS_SDW_ATTACH_TIMEOUT_MS	5000
+
+static int class_read_prop(struct sdw_slave *sdw)
+{
+	struct sdw_slave_prop *prop = &sdw->prop;
+
+	sdw_slave_read_prop(sdw);
+
+	prop->use_domain_irq = true;
+	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
+			      SDW_SCP_INT1_IMPL_DEF;
+
+	return 0;
+}
+
+static int class_sdw_update_status(struct sdw_slave *sdw, enum sdw_slave_status status)
+{
+	struct sdca_class_drv *drv = dev_get_drvdata(&sdw->dev);
+
+	switch (status) {
+	case SDW_SLAVE_ATTACHED:
+		dev_info(drv->dev, "device attach\n");
+
+		drv->attached = true;
+
+		complete(&drv->device_attach);
+		break;
+	case SDW_SLAVE_UNATTACHED:
+		dev_info(drv->dev, "device detach\n");
+
+		drv->attached = false;
+
+		reinit_completion(&drv->device_attach);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static const struct sdw_slave_ops class_sdw_ops = {
+	.read_prop	= class_read_prop,
+	.update_status	= class_sdw_update_status,
+};
+
+static void class_regmap_lock(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_lock(lock);
+}
+
+static void class_regmap_unlock(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_unlock(lock);
+}
+
+static int class_wait_for_attach(struct sdca_class_drv *drv)
+{
+	if (!drv->attached) {
+		unsigned long timeout = msecs_to_jiffies(CLASS_SDW_ATTACH_TIMEOUT_MS);
+		unsigned long time;
+
+		time = wait_for_completion_timeout(&drv->device_attach, timeout);
+		if (!time) {
+			dev_err(drv->dev, "timed out waiting for device re-attach\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	regcache_cache_only(drv->dev_regmap, false);
+
+	return 0;
+}
+
+static bool class_dev_regmap_volatile(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SDW_SCP_SDCA_INTMASK1 ... SDW_SCP_SDCA_INTMASK4:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static bool class_dev_regmap_precious(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case SDW_SCP_SDCA_INT1 ... SDW_SCP_SDCA_INT4:
+	case SDW_SCP_SDCA_INTMASK1 ... SDW_SCP_SDCA_INTMASK4:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct regmap_config class_dev_regmap_config = {
+	.name			= "sdca-device",
+	.reg_bits		= 32,
+	.val_bits		= 8,
+
+	.max_register		= SDW_SDCA_MAX_REGISTER,
+	.volatile_reg		= class_dev_regmap_volatile,
+	.precious_reg		= class_dev_regmap_precious,
+
+	.cache_type		= REGCACHE_MAPLE,
+
+	.lock			= class_regmap_lock,
+	.unlock			= class_regmap_unlock,
+};
+
+static void class_boot_work(struct work_struct *work)
+{
+	struct sdca_class_drv *drv = container_of(work,
+						  struct sdca_class_drv,
+						  boot_work);
+	int ret;
+
+	ret = class_wait_for_attach(drv);
+	if (ret)
+		goto err;
+
+	drv->irq_info = sdca_irq_allocate(drv->dev, drv->dev_regmap,
+					  drv->sdw->irq);
+	if (IS_ERR(drv->irq_info))
+		goto err;
+
+	ret = sdca_dev_register_functions(drv->sdw);
+	if (ret)
+		goto err;
+
+	dev_dbg(drv->dev, "boot work complete\n");
+
+	pm_runtime_mark_last_busy(drv->dev);
+	pm_runtime_put_autosuspend(drv->dev);
+
+	return;
+
+err:
+	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;
+	struct sdca_device_data *data = &sdw->sdca_data;
+	struct regmap_config *dev_config;
+	struct sdca_class_drv *drv;
+	int ret;
+
+	sdca_lookup_swft(sdw);
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	dev_config = devm_kmemdup(dev, &class_dev_regmap_config,
+				  sizeof(*dev_config), GFP_KERNEL);
+	if (!dev_config)
+		return -ENOMEM;
+
+	drv->functions = devm_kcalloc(dev, data->num_functions,
+				      sizeof(*drv->functions),
+				      GFP_KERNEL);
+	if (!drv->functions)
+		return -ENOMEM;
+
+	drv->dev = dev;
+	drv->sdw = sdw;
+	mutex_init(&drv->regmap_lock);
+
+	dev_set_drvdata(drv->dev, drv);
+
+	INIT_WORK(&drv->boot_work, class_boot_work);
+	init_completion(&drv->device_attach);
+
+	dev_config->lock_arg = &drv->regmap_lock;
+
+	drv->dev_regmap = devm_regmap_init_sdw(sdw, dev_config);
+	if (IS_ERR(drv->dev_regmap))
+		return dev_err_probe(drv->dev, PTR_ERR(drv->dev_regmap),
+				     "failed to create device regmap\n");
+
+	regcache_cache_only(drv->dev_regmap, true);
+
+	pm_runtime_set_autosuspend_delay(dev, 250);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+
+	ret = devm_pm_runtime_enable(dev);
+	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 int class_runtime_suspend(struct device *dev)
+{
+	struct sdca_class_drv *drv = dev_get_drvdata(dev);
+
+	/*
+	 * Whilst the driver doesn't power the chip down here, going into runtime
+	 * suspend lets the SoundWire bus power down, which means the driver
+	 * can't communicate with the device any more.
+	 */
+	regcache_cache_only(drv->dev_regmap, true);
+
+	return 0;
+}
+
+static int class_runtime_resume(struct device *dev)
+{
+	struct sdca_class_drv *drv = dev_get_drvdata(dev);
+	int ret;
+
+	ret = class_wait_for_attach(drv);
+	if (ret)
+		goto err;
+
+	regcache_mark_dirty(drv->dev_regmap);
+
+	ret = regcache_sync(drv->dev_regmap);
+	if (ret) {
+		dev_err(drv->dev, "failed to restore cache: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	regcache_cache_only(drv->dev_regmap, true);
+
+	return ret;
+}
+
+static const struct dev_pm_ops class_pm_ops = {
+	RUNTIME_PM_OPS(class_runtime_suspend, class_runtime_resume, NULL)
+};
+
+static const struct sdw_device_id class_sdw_id[] = {
+	SDW_SLAVE_ENTRY(0x01FA, 0x4245, 0),
+	{}
+};
+MODULE_DEVICE_TABLE(sdw, class_sdw_id);
+
+static struct sdw_driver class_sdw_driver = {
+	.driver = {
+		.name		= "sdca_class",
+		.pm		= pm_ptr(&class_pm_ops),
+	},
+
+	.probe		= class_sdw_probe,
+	.id_table	= class_sdw_id,
+	.ops		= &class_sdw_ops,
+};
+module_sdw_driver(class_sdw_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SDCA Class Driver");
+MODULE_IMPORT_NS("SND_SOC_SDCA");
diff --git a/sound/soc/sdca/sdca_class.h b/sound/soc/sdca/sdca_class.h
new file mode 100644
index 0000000000000..bb4c9dd124296
--- /dev/null
+++ b/sound/soc/sdca/sdca_class.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * The MIPI SDCA specification is available for public downloads at
+ * https://www.mipi.org/mipi-sdca-v1-0-download
+ *
+ * Copyright (C) 2025 Cirrus Logic, Inc. and
+ *                    Cirrus Logic International Semiconductor Ltd.
+ */
+
+#ifndef __SDCA_CLASS_H__
+#define __SDCA_CLASS_H__
+
+#include <linux/completion.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+
+struct device;
+struct regmap;
+struct sdw_slave;
+struct sdca_function_data;
+
+struct sdca_class_drv {
+	struct device *dev;
+	struct regmap *dev_regmap;
+	struct sdw_slave *sdw;
+
+	struct sdca_function_data *functions;
+	struct sdca_interrupt_info *irq_info;
+
+	struct mutex regmap_lock;
+	struct work_struct boot_work;
+	struct completion device_attach;
+
+	bool attached;
+};
+
+#endif /* __SDCA_CLASS_H__ */
-- 
2.47.3

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

> diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
> index e7f36d668f159..56e3ff8448762 100644
> --- a/sound/soc/sdca/Kconfig
> +++ b/sound/soc/sdca/Kconfig
> @@ -37,4 +37,14 @@ config SND_SOC_SDCA_FDL
>  config SND_SOC_SDCA_OPTIONAL
>  	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
>  
> +config SND_SOC_SDCA_CLASS
> +	tristate "SDCA Class Driver"
> +	select SND_SOC_SDCA
> +	select SND_SOC_SDCA_FDL

which builds on my other comment that the default y is probably not required.
config SND_SOC_SDCA_FDL
	bool "SDCA FDL (File DownLoad) support"
	depends on SND_SOC_SDCA
	default y

> +	select SND_SOC_SDCA_HID
> +	select SND_SOC_SDCA_IRQ
> +	help
> +	  This option enables support for the SDCA Class driver which should
> +	  support any class compliant SDCA part.
> +

> +#define CLASS_SDW_ATTACH_TIMEOUT_MS	5000

maybe add a comment on what the 'ATTACH' refers to.

> +
> +static int class_read_prop(struct sdw_slave *sdw)
> +{
> +	struct sdw_slave_prop *prop = &sdw->prop;
> +
> +	sdw_slave_read_prop(sdw);
> +
> +	prop->use_domain_irq = true;
> +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
> +			      SDW_SCP_INT1_IMPL_DEF;

Oh now I understand this 'class' driver is really a SoundWire device driver. You should clarify that this isn't meant to deal with function devices but it's one level up.

> +
> +	return 0;
> +}
> +
> +static int class_sdw_update_status(struct sdw_slave *sdw, enum sdw_slave_status status)
> +{
> +	struct sdca_class_drv *drv = dev_get_drvdata(&sdw->dev);
> +
> +	switch (status) {
> +	case SDW_SLAVE_ATTACHED:
> +		dev_info(drv->dev, "device attach\n");

probably too verbose, dev_dbg() would make more sense IMHO.

> +
> +		drv->attached = true;
> +
> +		complete(&drv->device_attach);
> +		break;
> +	case SDW_SLAVE_UNATTACHED:
> +		dev_info(drv->dev, "device detach\n");
> +
> +		drv->attached = false;
> +
> +		reinit_completion(&drv->device_attach);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct sdw_slave_ops class_sdw_ops = {
> +	.read_prop	= class_read_prop,
> +	.update_status	= class_sdw_update_status,
> +};
> +
> +static void class_regmap_lock(void *data)
> +{
> +	struct mutex *lock = data;
> +
> +	mutex_lock(lock);
> +}
> +
> +static void class_regmap_unlock(void *data)
> +{
> +	struct mutex *lock = data;
> +
> +	mutex_unlock(lock);
> +}
> +
> +static int class_wait_for_attach(struct sdca_class_drv *drv)
> +{
> +	if (!drv->attached) {
> +		unsigned long timeout = msecs_to_jiffies(CLASS_SDW_ATTACH_TIMEOUT_MS);
> +		unsigned long time;
> +
> +		time = wait_for_completion_timeout(&drv->device_attach, timeout);
> +		if (!time) {
> +			dev_err(drv->dev, "timed out waiting for device re-attach\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	regcache_cache_only(drv->dev_regmap, false);
> +
> +	return 0;
> +}
> +
> +static bool class_dev_regmap_volatile(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case SDW_SCP_SDCA_INTMASK1 ... SDW_SCP_SDCA_INTMASK4:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
> +static bool class_dev_regmap_precious(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case SDW_SCP_SDCA_INT1 ... SDW_SCP_SDCA_INT4:
> +	case SDW_SCP_SDCA_INTMASK1 ... SDW_SCP_SDCA_INTMASK4:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
> +static const struct regmap_config class_dev_regmap_config = {
> +	.name			= "sdca-device",
> +	.reg_bits		= 32,
> +	.val_bits		= 8,
> +
> +	.max_register		= SDW_SDCA_MAX_REGISTER,
> +	.volatile_reg		= class_dev_regmap_volatile,
> +	.precious_reg		= class_dev_regmap_precious,
> +
> +	.cache_type		= REGCACHE_MAPLE,
> +
> +	.lock			= class_regmap_lock,
> +	.unlock			= class_regmap_unlock,
> +};
> +
> +static void class_boot_work(struct work_struct *work)
> +{
> +	struct sdca_class_drv *drv = container_of(work,
> +						  struct sdca_class_drv,
> +						  boot_work);
> +	int ret;
> +
> +	ret = class_wait_for_attach(drv);
> +	if (ret)
> +		goto err;
> +
> +	drv->irq_info = sdca_irq_allocate(drv->dev, drv->dev_regmap,
> +					  drv->sdw->irq);
> +	if (IS_ERR(drv->irq_info))
> +		goto err;
> +
> +	ret = sdca_dev_register_functions(drv->sdw);
> +	if (ret)
> +		goto err;
> +
> +	dev_dbg(drv->dev, "boot work complete\n");
> +
> +	pm_runtime_mark_last_busy(drv->dev);
> +	pm_runtime_put_autosuspend(drv->dev);
> +
> +	return;
> +
> +err:
> +	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;
> +	struct sdca_device_data *data = &sdw->sdca_data;
> +	struct regmap_config *dev_config;
> +	struct sdca_class_drv *drv;
> +	int ret;
> +
> +	sdca_lookup_swft(sdw);
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	dev_config = devm_kmemdup(dev, &class_dev_regmap_config,
> +				  sizeof(*dev_config), GFP_KERNEL);
> +	if (!dev_config)
> +		return -ENOMEM;
> +
> +	drv->functions = devm_kcalloc(dev, data->num_functions,
> +				      sizeof(*drv->functions),
> +				      GFP_KERNEL);
> +	if (!drv->functions)
> +		return -ENOMEM;
> +
> +	drv->dev = dev;
> +	drv->sdw = sdw;
> +	mutex_init(&drv->regmap_lock);
> +
> +	dev_set_drvdata(drv->dev, drv);
> +
> +	INIT_WORK(&drv->boot_work, class_boot_work);
> +	init_completion(&drv->device_attach);
> +
> +	dev_config->lock_arg = &drv->regmap_lock;
> +
> +	drv->dev_regmap = devm_regmap_init_sdw(sdw, dev_config);
> +	if (IS_ERR(drv->dev_regmap))
> +		return dev_err_probe(drv->dev, PTR_ERR(drv->dev_regmap),
> +				     "failed to create device regmap\n");
> +
> +	regcache_cache_only(drv->dev_regmap, true);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 250);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);

This seems odd, I don't recall that we do something similar in existing codec drivers.
It'd be worth explaining the whole pm_runtime flow.

> +
> +	ret = devm_pm_runtime_enable(dev);
> +	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 int class_runtime_suspend(struct device *dev)
> +{
> +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> +
> +	/*
> +	 * Whilst the driver doesn't power the chip down here, going into runtime
> +	 * suspend lets the SoundWire bus power down, which means the driver
> +	 * can't communicate with the device any more.
> +	 */
> +	regcache_cache_only(drv->dev_regmap, true);
> +
> +	return 0;
> +}
> +
> +static int class_runtime_resume(struct device *dev)
> +{
> +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = class_wait_for_attach(drv);

You probably need to explain the model here. In the general case a bus could just enable clock_stop and the attach should be immediate on restarting the clock. The wait only makes sense for cases where the bus is powered-down. Not all solutions will choose the last option.

> +	if (ret)
> +		goto err;
> +
> +	regcache_mark_dirty(drv->dev_regmap);
> +
> +	ret = regcache_sync(drv->dev_regmap);
> +	if (ret) {
> +		dev_err(drv->dev, "failed to restore cache: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	regcache_cache_only(drv->dev_regmap, true);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops class_pm_ops = {
> +	RUNTIME_PM_OPS(class_runtime_suspend, class_runtime_resume, NULL)
> +};
> +
> +static const struct sdw_device_id class_sdw_id[] = {
> +	SDW_SLAVE_ENTRY(0x01FA, 0x4245, 0),

It seems odd to put specific ACPI identifiers here.
It also begs the question on how extensions could be managed.
For a first-start it's probably ok but we've got to have alignment on how we identify 
- devices for which the default class is 'good-enough'
- devices where an extension on top of the default class is required
- devices where a custom driver is needed

IIRC when I looked at class drivers a long time ago, we don't have the plumbing in the SoundWire bus to automagically load a class driver if no custom driver is found - and adding such plumbing was way above my skills/knowledge.

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Mon, Oct 27, 2025 at 04:02:54PM +0100, Pierre-Louis Bossart wrote:
> > diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
> > index e7f36d668f159..56e3ff8448762 100644
> > --- a/sound/soc/sdca/Kconfig
> > +++ b/sound/soc/sdca/Kconfig
> > @@ -37,4 +37,14 @@ config SND_SOC_SDCA_FDL
> >  config SND_SOC_SDCA_OPTIONAL
> >  	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
> >  
> > +config SND_SOC_SDCA_CLASS
> > +	tristate "SDCA Class Driver"
> > +	select SND_SOC_SDCA
> > +	select SND_SOC_SDCA_FDL
> 
> which builds on my other comment that the default y is probably not required.
> config SND_SOC_SDCA_FDL
> 	bool "SDCA FDL (File DownLoad) support"
> 	depends on SND_SOC_SDCA
> 	default y

Yeah ok I see what you are saying here, I would be reticent to
remove these selects because the driver depends on those
features, but then if all drivers think like that the default
does become redundant.

> > +	select SND_SOC_SDCA_HID
> > +	select SND_SOC_SDCA_IRQ
> > +	help
> > +	  This option enables support for the SDCA Class driver which should
> > +	  support any class compliant SDCA part.
> > +
> 
> > +#define CLASS_SDW_ATTACH_TIMEOUT_MS	5000
> 
> maybe add a comment on what the 'ATTACH' refers to.

Is this really ambiguous attach has a pretty defined meaning in a
soundwire context and it is SDW_ATTACH?

> > +
> > +static int class_read_prop(struct sdw_slave *sdw)
> > +{
> > +	struct sdw_slave_prop *prop = &sdw->prop;
> > +
> > +	sdw_slave_read_prop(sdw);
> > +
> > +	prop->use_domain_irq = true;
> > +	prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY |
> > +			      SDW_SCP_INT1_IMPL_DEF;
> 
> Oh now I understand this 'class' driver is really a SoundWire
> device driver. You should clarify that this isn't meant to deal
> with function devices but it's one level up.

Yeah the commit message in general needs some beefing up.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int class_sdw_update_status(struct sdw_slave *sdw, enum sdw_slave_status status)
> > +{
> > +	struct sdca_class_drv *drv = dev_get_drvdata(&sdw->dev);
> > +
> > +	switch (status) {
> > +	case SDW_SLAVE_ATTACHED:
> > +		dev_info(drv->dev, "device attach\n");
> 
> probably too verbose, dev_dbg() would make more sense IMHO.

Agree yeah makes sense.

> > +	pm_runtime_set_autosuspend_delay(dev, 250);
> > +	pm_runtime_use_autosuspend(dev);
> > +	pm_runtime_set_active(dev);
> > +	pm_runtime_get_noresume(dev);
> 
> This seems odd, I don't recall that we do something similar in existing codec drivers.
> It'd be worth explaining the whole pm_runtime flow.

I suspect that means those codec drivers are broken :-) at least
if they actively use pm_runtime. The first two enable
autosuspend, that is suspending the device after a short period
of inactivity. set_active tells the PM runtime the device is
already powered up, as it is. Finally, get_noresume takes a
reference, this will stop it powering down immediately once the
pm_runtime is enabled, it will waiting until we put the reference
we are holding.

> > +static int class_runtime_resume(struct device *dev)
> > +{
> > +	struct sdca_class_drv *drv = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = class_wait_for_attach(drv);
> 
> You probably need to explain the model here. In the general
> case a bus could just enable clock_stop and the attach should be
> immediate on restarting the clock. The wait only makes sense for
> cases where the bus is powered-down. Not all solutions will choose
> the last option.
> 

If the device never dettached wait_for_attach will just return
immediately. I am not sure exactly what the problem is here, the
soundwire framework in Linux does not guarantee the device is
attached before the runtime_resume callback is called so we need
to ensure that is the case. Now personally I would probably argue
it would be nice if the framework did guarantee that but this
isn't an SDCA thing, that is an existing property of the
soundwire stuff so I don't think blocking the SDCA on changing it
makes sense. But it is definitely something that people finding
time to work on would be awesome.

> > +static const struct sdw_device_id class_sdw_id[] = {
> > +	SDW_SLAVE_ENTRY(0x01FA, 0x4245, 0),
> 
> It seems odd to put specific ACPI identifiers here.

This is likely not the full complete solution but probably is a
reasonable place to start for now.

> It also begs the question on how extensions could be managed.
> For a first-start it's probably ok but we've got to have alignment on
> how we identify 
> - devices for which the default class is 'good-enough'

Here we identify those by which identifiers are added to the
class driver.

> - devices where an extension on top of the default class is required
> - devices where a custom driver is needed

Currently the philosophy we are working on is that there are no
extensions which I thought we had discussed in the past.  All the
SDCA stuff has been implemented as a library of helpers, that
means that if your part needs custom handling you should
implement a custom driver but for the parts where you are spec
compliant you can just call the library helpers so you don't have
to reimplement those bits. The class driver here is just some
code that calls the superset of helpers.

> IIRC when I looked at class drivers a long time ago, we don't
> have the plumbing in the SoundWire bus to automagically load a class
> driver if no custom driver is found - and adding such plumbing was
> way above my skills/knowledge.

Yeah long term something like check for driver match then if not
check the SDCA bit and try the class driver if it is set. But I
don't think those are problems we should be trying to solve here.
Nothing here really forces any choices on these decisions later.

Thanks,
Charles

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Richard Fitzgerald]

On 30/10/2025 3:29 pm, Charles Keepax wrote:
> On Mon, Oct 27, 2025 at 04:02:54PM +0100, Pierre-Louis Bossart wrote:

SNIP

>>> +	pm_runtime_set_autosuspend_delay(dev, 250);
>>> +	pm_runtime_use_autosuspend(dev);
>>> +	pm_runtime_set_active(dev);
>>> +	pm_runtime_get_noresume(dev);
>>
>> This seems odd, I don't recall that we do something similar in existing codec drivers.
>> It'd be worth explaining the whole pm_runtime flow.
> 
> I suspect that means those codec drivers are broken :-) at least
> if they actively use pm_runtime. The first two enable
> autosuspend, that is suspending the device after a short period
> of inactivity. set_active tells the PM runtime the device is
> already powered up, as it is. Finally, get_noresume takes a
> reference, this will stop it powering down immediately once the
> pm_runtime is enabled, it will waiting until we put the reference
> we are holding.
> 
Other drivers definitely do that.
For example:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/rt722-sdca.c#n1544

(the other rt drivers are similar)

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

On 10/30/25 16:36, Richard Fitzgerald wrote:
> On 30/10/2025 3:29 pm, Charles Keepax wrote:
>> On Mon, Oct 27, 2025 at 04:02:54PM +0100, Pierre-Louis Bossart wrote:
> 
> SNIP
> 
>>>> +    pm_runtime_set_autosuspend_delay(dev, 250);
>>>> +    pm_runtime_use_autosuspend(dev);
>>>> +    pm_runtime_set_active(dev);
>>>> +    pm_runtime_get_noresume(dev);
>>>
>>> This seems odd, I don't recall that we do something similar in existing codec drivers.
>>> It'd be worth explaining the whole pm_runtime flow.
>>
>> I suspect that means those codec drivers are broken :-) at least
>> if they actively use pm_runtime. The first two enable
>> autosuspend, that is suspending the device after a short period
>> of inactivity. set_active tells the PM runtime the device is
>> already powered up, as it is. Finally, get_noresume takes a
>> reference, this will stop it powering down immediately once the
>> pm_runtime is enabled, it will waiting until we put the reference
>> we are holding.
>>
> Other drivers definitely do that.
> For example:
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/rt722-sdca.c#n1544
> 
> (the other rt drivers are similar)

This rt722-sdca driver seems wrong, IIRC we aligned all drivers to do most of the inits in the driver probe, and then the only thing left to do when the device attaches is the set_active/get_noresume/put_autosuspend.

See the rt711 and rt1308 for reference, they have comments to explain why we used this code pattern.

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/rt1308-sdw.c#n709

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Tue, Nov 04, 2025 at 05:13:39PM +0100, Pierre-Louis Bossart wrote:
> On 10/30/25 16:36, Richard Fitzgerald wrote:
> > On 30/10/2025 3:29 pm, Charles Keepax wrote:
> >> On Mon, Oct 27, 2025 at 04:02:54PM +0100, Pierre-Louis Bossart wrote:
> > 
> > SNIP
> > 
> >>>> +    pm_runtime_set_autosuspend_delay(dev, 250);
> >>>> +    pm_runtime_use_autosuspend(dev);
> >>>> +    pm_runtime_set_active(dev);
> >>>> +    pm_runtime_get_noresume(dev);
> >>>
> >>> This seems odd, I don't recall that we do something similar in existing codec drivers.
> >>> It'd be worth explaining the whole pm_runtime flow.
> >>
> >> I suspect that means those codec drivers are broken :-) at least
> >> if they actively use pm_runtime. The first two enable
> >> autosuspend, that is suspending the device after a short period
> >> of inactivity. set_active tells the PM runtime the device is
> >> already powered up, as it is. Finally, get_noresume takes a
> >> reference, this will stop it powering down immediately once the
> >> pm_runtime is enabled, it will waiting until we put the reference
> >> we are holding.
> >>
> > Other drivers definitely do that.
> > For example:
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/rt722-sdca.c#n1544
> > 
> > (the other rt drivers are similar)
> 
> This rt722-sdca driver seems wrong, IIRC we aligned all
> drivers to do most of the inits in the driver probe, and then
> the only thing left to do when the device attaches is the
> set_active/get_noresume/put_autosuspend.
> 
> See the rt711 and rt1308 for reference, they have comments to
> explain why we used this code pattern.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/rt1308-sdw.c#n709

That is some scary stuff there, that is basically working around
the fact that with those drivers the soundcard is created before
the hardware is actually ready. But its one aspect of that and
there are likely many knock on effects/races hiding in there. At
some point we should probably revisit the whole enumeration
thing in soundwire it does cause a lot of scary code.

That said... the class driver doesn't have the same problem
however, because of the two layer nature of the auxiliary driver
stuff. The soundwire driver binds to the device and completes
probe, but it is the auxiliary drivers that are used in the
soundcard and those are only probed once the device itself has
enumerated in class_boot_work(). This means the sound card is
only created after the device has enumerated, so the same problem
isn't present and we can have a more normal PM runtime startup.

Thanks,
Charles

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

> That is some scary stuff there, that is basically working around
> the fact that with those drivers the soundcard is created before
> the hardware is actually ready. But its one aspect of that and
> there are likely many knock on effects/races hiding in there. At
> some point we should probably revisit the whole enumeration
> thing in soundwire it does cause a lot of scary code.
I don't see how we can revisit this, the codec probe happens based on ACPI information even before the bus is started. The card driver is also probed when the PCI driver creates a platform device.

> That said... the class driver doesn't have the same problem
> however, because of the two layer nature of the auxiliary driver
> stuff. The soundwire driver binds to the device and completes
> probe, but it is the auxiliary drivers that are used in the
> soundcard and those are only probed once the device itself has
> enumerated in class_boot_work(). This means the sound card is
> only created after the device has enumerated, so the same problem
> isn't present and we can have a more normal PM runtime startup.Humm, that's an important detail indeed that I completely missed...

You could also have registered the function subdevices based on ACPI information *before* the whole enumeration. I can see why you took that function-register-after-device-enumeration route, but I have mixed feelings about having separate mechanisms for vendor- and class-drivers.  

Note that things will be interesting when we use the new ACPI aggregation information to create the card, we're missing a means to re-trigger deferred probe checks as devices become functional. I had a patch on this a couple of years ago, look for "driver core: export driver_deferred_probe_trigger()", we probably need to revisit the entire scheme...

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
> > That is some scary stuff there, that is basically working around
> > the fact that with those drivers the soundcard is created before
> > the hardware is actually ready. But its one aspect of that and
> > there are likely many knock on effects/races hiding in there. At
> > some point we should probably revisit the whole enumeration
> > thing in soundwire it does cause a lot of scary code.
> I don't see how we can revisit this, the codec probe happens
> based on ACPI information even before the bus is started. The
> card driver is also probed when the PCI driver creates a platform
> device.

I mean in general the fact SoundWire probes devices before they
are available. It introduces a lot of scary things, as basically
the whole kernel is setup to believe that when a device is probed
it is ready. I mean I know why we did it, because we might need
to setup some things (resets/regulators) to cause the SoundWire
device to enumerate, but it does result in scaryness.

> > That said... the class driver doesn't have the same problem
> > however, because of the two layer nature of the auxiliary driver
> > stuff. The soundwire driver binds to the device and completes
> > probe, but it is the auxiliary drivers that are used in the
> > soundcard and those are only probed once the device itself has
> > enumerated in class_boot_work(). This means the sound card is
> > only created after the device has enumerated, so the same problem
> > isn't present and we can have a more normal PM runtime startup.
> Humm, that's an important detail indeed that I completely missed...
> 
> You could also have registered the function subdevices based on
> ACPI information *before* the whole enumeration. I can see why
> you took that function-register-after-device-enumeration route,
> but I have mixed feelings about having separate mechanisms for
> vendor- and class-drivers.

At least currently you have separate mechanisms regardless
of the choice of probe time since all existing vendor drivers
register a single driver and the class driver registers many
through auxiliary. The difference is only in the secondary
drivers that the vendor drivers don't have anyway.

Personally I like the only registering the functions when the
device has enumerated approach as keeps interactions with other
kernel subsystems within the expectations those subsystems have
of the driver. There is a lot less to think about.

Ultimately, I think the long game solution here is probably that
we move SoundWire to internally have two devices. An initial
device the gets registered and does the probe setup and then a
child device that probes on enumeration. Basically, the same
setup as we have for the auxiliaries in SDCA but internal to
SoundWire. But in the mean time I think using MFD/auxiliary to
introduce a 2 step probe seems like a really neat solution.

Additionally, there arn't that many SDCA vendor drivers at the
moment, a handful of Realtek ones and a TI one. If we wanted
to standardise I would suggest standardising on the better
solution, which to my mind is clearly this approach.

> Note that things will be interesting when we use the new
> ACPI aggregation information to create the card, we're missing
> a means to re-trigger deferred probe checks as devices become
> functional. I had a patch on this a couple of years ago, look
> for "driver core: export driver_deferred_probe_trigger()",
> we probably need to revisit the entire scheme...

Apologies if I am misunderstanding but is this not another example
of the advantages of probing when the device enumerates? If I
understand correctly the problem those patches are solving is
resources that become available outside of a probe break deferred
probe. By tying the probe of the auxiliaries to the enumeration
of the device, we ensure that link between probe and resource
availability. I think the point Greg is making in that thread
is that the kernel expects resources are available once probed,
so if one is going to break that assumption it should really
be handled exclusively in the driver that does that. But the
simplest solution to my mind is just to not break that assumption
then everything works as expected.

Thanks,
Charles

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

On 12/10/25 10:55, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
>>> That is some scary stuff there, that is basically working around
>>> the fact that with those drivers the soundcard is created before
>>> the hardware is actually ready. But its one aspect of that and
>>> there are likely many knock on effects/races hiding in there. At
>>> some point we should probably revisit the whole enumeration
>>> thing in soundwire it does cause a lot of scary code.
>> I don't see how we can revisit this, the codec probe happens
>> based on ACPI information even before the bus is started. The
>> card driver is also probed when the PCI driver creates a platform
>> device.
> 
> I mean in general the fact SoundWire probes devices before they
> are available. It introduces a lot of scary things, as basically
> the whole kernel is setup to believe that when a device is probed
> it is ready. I mean I know why we did it, because we might need
> to setup some things (resets/regulators) to cause the SoundWire
> device to enumerate, but it does result in scaryness.
> 
>>> That said... the class driver doesn't have the same problem
>>> however, because of the two layer nature of the auxiliary driver
>>> stuff. The soundwire driver binds to the device and completes
>>> probe, but it is the auxiliary drivers that are used in the
>>> soundcard and those are only probed once the device itself has
>>> enumerated in class_boot_work(). This means the sound card is
>>> only created after the device has enumerated, so the same problem
>>> isn't present and we can have a more normal PM runtime startup.
>> Humm, that's an important detail indeed that I completely missed...
>>
>> You could also have registered the function subdevices based on
>> ACPI information *before* the whole enumeration. I can see why
>> you took that function-register-after-device-enumeration route,
>> but I have mixed feelings about having separate mechanisms for
>> vendor- and class-drivers.
> 
> At least currently you have separate mechanisms regardless
> of the choice of probe time since all existing vendor drivers
> register a single driver and the class driver registers many
> through auxiliary. The difference is only in the secondary
> drivers that the vendor drivers don't have anyway.
> 
> Personally I like the only registering the functions when the
> device has enumerated approach as keeps interactions with other
> kernel subsystems within the expectations those subsystems have
> of the driver. There is a lot less to think about.
> 
> Ultimately, I think the long game solution here is probably that
> we move SoundWire to internally have two devices. An initial
> device the gets registered and does the probe setup and then a
> child device that probes on enumeration. Basically, the same
> setup as we have for the auxiliaries in SDCA but internal to
> SoundWire. But in the mean time I think using MFD/auxiliary to
> introduce a 2 step probe seems like a really neat solution.
> 
> Additionally, there arn't that many SDCA vendor drivers at the
> moment, a handful of Realtek ones and a TI one. If we wanted
> to standardise I would suggest standardising on the better
> solution, which to my mind is clearly this approach.

I am not against having a single mechanism, and that change would be relatively minimal.

The only issue I have with it is whether one would deregister the child device on a soft reset or whenever the device loses sync? It'd be somewhat ugly to do so, but then again we have the issue that for the second enumeration we already have a probed child driver, which would bring us back to an async model really.
The 'beauty' of the current model is that the probe does nothing really, everything happens at enumeration/sync loss. If we dynamically add/remove child devices it'll be 'fun' really quickly.

>> Note that things will be interesting when we use the new
>> ACPI aggregation information to create the card, we're missing
>> a means to re-trigger deferred probe checks as devices become
>> functional. I had a patch on this a couple of years ago, look
>> for "driver core: export driver_deferred_probe_trigger()",
>> we probably need to revisit the entire scheme...
> 
> Apologies if I am misunderstanding but is this not another example
> of the advantages of probing when the device enumerates? If I
> understand correctly the problem those patches are solving is
> resources that become available outside of a probe break deferred
> probe. By tying the probe of the auxiliaries to the enumeration
> of the device, we ensure that link between probe and resource
> availability. I think the point Greg is making in that thread
> is that the kernel expects resources are available once probed,
> so if one is going to break that assumption it should really
> be handled exclusively in the driver that does that. But the
> simplest solution to my mind is just to not break that assumption
> then everything works as expected.

That's precisely the problem, the model assumes something that is broken left and right in practice.
Exhibit A is the PCI/HDaudio parts where we rely on an async probe due to the module handling.
You also have devices that require firmware download to be functional, or some sort of internal ramp-up needed.
Assuming a perfect behavior where all resources are available at probe time is naive IMHO. In the specific case of the machine driver probed with ACPI information that isn't tied to the bus status, it's a guaranteed fail.

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Sat, Dec 20, 2025 at 12:04:47PM +0100, Pierre-Louis Bossart wrote:
> On 12/10/25 10:55, Charles Keepax wrote:
> > On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
> > Additionally, there arn't that many SDCA vendor drivers at the
> > moment, a handful of Realtek ones and a TI one. If we wanted
> > to standardise I would suggest standardising on the better
> > solution, which to my mind is clearly this approach.
> 
> I am not against having a single mechanism, and that change
> would be relatively minimal.
> 
> The only issue I have with it is whether one would deregister
> the child device on a soft reset or whenever the device
> loses sync? It'd be somewhat ugly to do so, but then again
> we have the issue that for the second enumeration we already
> have a probed child driver, which would bring us back to an
> async model really. The 'beauty' of the current model is
> that the probe does nothing really, everything happens at
> enumeration/sync loss. If we dynamically add/remove child
> devices it'll be 'fun' really quickly.

Yeah that is one of the questions I have pondered a few times.
Were I usually get to is separating out expected cases of
something dropping off the bus and unexpected cases. For say
suspend/resume it is quite likely the device will drop off the
bus and that is expected and probably shouldn't result in the
child drivers being removed, as it will as you say become 'fun'
very quickly. However, an unexpected drop off the bus during
normal operation is really a pretty fatal error. In many ways the
best thing to do is probably to remove the child drivers in this
case, since that will tear down the sound card, and allow
everything to be rebuilt. But these are far from fully formed
ideas, for the most part at the moment we just report things went
bad.

> >> Note that things will be interesting when we use the new
> >> ACPI aggregation information to create the card, we're missing
> >> a means to re-trigger deferred probe checks as devices become
> >> functional. I had a patch on this a couple of years ago, look
> >> for "driver core: export driver_deferred_probe_trigger()",
> >> we probably need to revisit the entire scheme...
> > 
> > Apologies if I am misunderstanding but is this not another example
> > of the advantages of probing when the device enumerates? If I
> > understand correctly the problem those patches are solving is
> > resources that become available outside of a probe break deferred
> > probe. By tying the probe of the auxiliaries to the enumeration
> > of the device, we ensure that link between probe and resource
> > availability. I think the point Greg is making in that thread
> > is that the kernel expects resources are available once probed,
> > so if one is going to break that assumption it should really
> > be handled exclusively in the driver that does that. But the
> > simplest solution to my mind is just to not break that assumption
> > then everything works as expected.
> 
> That's precisely the problem, the model assumes something
> that is broken left and right in practice.
> Exhibit A is the PCI/HDaudio parts where we rely on an async
> probe due to the module handling.

I think I am perhaps not totally familiar with the issues on the
PCI/HDaudio side. What is the primary issue here?

> You also have devices that require firmware download to be
> functional, or some sort of internal ramp-up needed.

Yeah these can be fairly annoying. But both of these can be
tackled with either handling the situation in probe, or if that
makes boot too slow, moving to a similar child driver approach.

> Assuming a perfect behavior where all resources are available
> at probe time is naive IMHO. In the specific case of the
> machine driver probed with ACPI information that isn't tied
> to the bus status, it's a guaranteed fail.

Well I do love a good bit of being naive until it bites me :-)
Ultimately the machine driver has to call snd_soc_register_card
which can probe defer if the card components aren't ready. What
parts cause the issue here?

Thanks,
Charles

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

On 1/6/26 13:58, Charles Keepax wrote:
> On Sat, Dec 20, 2025 at 12:04:47PM +0100, Pierre-Louis Bossart wrote:
>> On 12/10/25 10:55, Charles Keepax wrote:
>>> On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
>>> Additionally, there arn't that many SDCA vendor drivers at the
>>> moment, a handful of Realtek ones and a TI one. If we wanted
>>> to standardise I would suggest standardising on the better
>>> solution, which to my mind is clearly this approach.
>>
>> I am not against having a single mechanism, and that change
>> would be relatively minimal.
>>
>> The only issue I have with it is whether one would deregister
>> the child device on a soft reset or whenever the device
>> loses sync? It'd be somewhat ugly to do so, but then again
>> we have the issue that for the second enumeration we already
>> have a probed child driver, which would bring us back to an
>> async model really. The 'beauty' of the current model is
>> that the probe does nothing really, everything happens at
>> enumeration/sync loss. If we dynamically add/remove child
>> devices it'll be 'fun' really quickly.
> 
> Yeah that is one of the questions I have pondered a few times.
> Were I usually get to is separating out expected cases of
> something dropping off the bus and unexpected cases. For say
> suspend/resume it is quite likely the device will drop off the
> bus and that is expected and probably shouldn't result in the
> child drivers being removed, as it will as you say become 'fun'
> very quickly. However, an unexpected drop off the bus during
> normal operation is really a pretty fatal error. In many ways the
> best thing to do is probably to remove the child drivers in this
> case, since that will tear down the sound card, and allow
> everything to be rebuilt. But these are far from fully formed
> ideas, for the most part at the moment we just report things went
> bad.

A device falling off the bus is not that rare in early bring-up or experimental platforms. 
IIRC some devices will require a full reset after firmware download.
Not to mention the glitches that can be seen consistently after clock stop on some platforms.

My take is that it's better to 'hide' some of the low-level detail and keep the card visible to apps, always. That means the drivers need to deal with cases where accesses to devices might be delayed due to slow sync or sync loss. As long as everything recovers we are good to go.

One way to test the recovery capabilities is to inject a bad sync pattern. The Cadence IP can do this manually, I vaguely remember adding a debugfs entry to test this but I don't recall if it ever made it in the mainline.

>>>> Note that things will be interesting when we use the new
>>>> ACPI aggregation information to create the card, we're missing
>>>> a means to re-trigger deferred probe checks as devices become
>>>> functional. I had a patch on this a couple of years ago, look
>>>> for "driver core: export driver_deferred_probe_trigger()",
>>>> we probably need to revisit the entire scheme...
>>>
>>> Apologies if I am misunderstanding but is this not another example
>>> of the advantages of probing when the device enumerates? If I
>>> understand correctly the problem those patches are solving is
>>> resources that become available outside of a probe break deferred
>>> probe. By tying the probe of the auxiliaries to the enumeration
>>> of the device, we ensure that link between probe and resource
>>> availability. I think the point Greg is making in that thread
>>> is that the kernel expects resources are available once probed,
>>> so if one is going to break that assumption it should really
>>> be handled exclusively in the driver that does that. But the
>>> simplest solution to my mind is just to not break that assumption
>>> then everything works as expected.
>>
>> That's precisely the problem, the model assumes something
>> that is broken left and right in practice.
>> Exhibit A is the PCI/HDaudio parts where we rely on an async
>> probe due to the module handling.
> 
> I think I am perhaps not totally familiar with the issues on the
> PCI/HDaudio side. What is the primary issue here?

The HDAudio side loads different modules, which can take a lot of time. For that reason, the driver probe returns immediately but most of the processing is done in a workqueue. This leads to two problems
a) if the processing fails for some reason, we have no way of signaling any error.
b) if the card is created independently from the PCI driver, e.g. with ACPI information, then we have no way of telling that the resources needed by the card are available.

That b) point isn't an issue at the moment because the PCI driver creates a platform devices which results in the card creation by the platform driver. But if that sequential part is broken then the deferred probe mechanism will fail by construction - it assumes that all resources are available when the probe returns.
 
>> You also have devices that require firmware download to be
>> functional, or some sort of internal ramp-up needed.
> 
> Yeah these can be fairly annoying. But both of these can be
> tackled with either handling the situation in probe, or if that
> makes boot too slow, moving to a similar child driver approach.

Both options are not so good IMHO...

>> Assuming a perfect behavior where all resources are available
>> at probe time is naive IMHO. In the specific case of the
>> machine driver probed with ACPI information that isn't tied
>> to the bus status, it's a guaranteed fail.
> 
> Well I do love a good bit of being naive until it bites me :-)
> Ultimately the machine driver has to call snd_soc_register_card
> which can probe defer if the card components aren't ready. What
> parts cause the issue here?

The issue is that the deferred probe mechanism revisits the list of needed resources when a new device is added or when a probe completes. When the probe completes 'later', then we are missing a mechanism to signal that a missing resource is now available.

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Tue, Jan 06, 2026 at 06:10:06PM +0100, Pierre-Louis Bossart wrote:
> On 1/6/26 13:58, Charles Keepax wrote:
> > On Sat, Dec 20, 2025 at 12:04:47PM +0100, Pierre-Louis Bossart wrote:
> >> On 12/10/25 10:55, Charles Keepax wrote:
> >>> On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
> >> The only issue I have with it is whether one would deregister
> >> the child device on a soft reset or whenever the device
> >> loses sync? It'd be somewhat ugly to do so, but then again
> >> we have the issue that for the second enumeration we already
> >> have a probed child driver, which would bring us back to an
> >> async model really. The 'beauty' of the current model is
> >> that the probe does nothing really, everything happens at
> >> enumeration/sync loss. If we dynamically add/remove child
> >> devices it'll be 'fun' really quickly.
> > 
> > Yeah that is one of the questions I have pondered a few times.
> > Were I usually get to is separating out expected cases of
> > something dropping off the bus and unexpected cases. For say
> > suspend/resume it is quite likely the device will drop off the
> > bus and that is expected and probably shouldn't result in the
> > child drivers being removed, as it will as you say become 'fun'
> > very quickly. However, an unexpected drop off the bus during
> > normal operation is really a pretty fatal error. In many ways the
> > best thing to do is probably to remove the child drivers in this
> > case, since that will tear down the sound card, and allow
> > everything to be rebuilt. But these are far from fully formed
> > ideas, for the most part at the moment we just report things went
> > bad.
> 
> A device falling off the bus is not that rare in early
> bring-up or experimental platforms.  IIRC some devices will
> require a full reset after firmware download.  Not to mention
> the glitches that can be seen consistently after clock stop
> on some platforms.

So doing a reset after firmware download should be fine, I would
consider than an expected drop off.

> My take is that it's better to 'hide' some of the low-level
> detail and keep the card visible to apps, always.

I would agree here, it is better if the card stays present and
hides non-error states.

> That means the drivers need to deal with cases where accesses
> to devices might be delayed due to slow sync or sync loss. As
> long as everything recovers we are good to go.

Yeah that is a point we slightly disagree on, I really am not
a huge fan of auto-recovery stuff. Been involved in too many
situations where the auto-recovery recovers *most* of the time
and takes a 1/10 fail to a 1/10000 fail.

Taking this case, if the device loses sync during audio that
is gonna glitch the audio, which makes it a problem I will be
expected to fix. I would rather get a report the audio stopped
with a log up to that point, than get reports of audio glitches.

> >> That's precisely the problem, the model assumes something
> >> that is broken left and right in practice.
> >> Exhibit A is the PCI/HDaudio parts where we rely on an async
> >> probe due to the module handling.
> > 
> > I think I am perhaps not totally familiar with the issues on the
> > PCI/HDaudio side. What is the primary issue here?
> 
> The HDAudio side loads different modules, which can take a lot
> of time. For that reason, the driver probe returns immediately
> but most of the processing is done in a workqueue. This leads
> to two problems
> a) if the processing fails for some reason, we have no way
>    of signaling any error.
> b) if the card is created independently from the PCI driver,
>    e.g. with ACPI information, then we have no way of telling
>    that the resources needed by the card are available.

I mean sure it does make the boot look faster, but is it really
faster as the devices weren't actually ready when we told
user-space they were? And now we have to deal with the problems
outlined here, as well as what happens if user-space/another
driver tries to use the device whilst it is in this
not-quite-ready state. It feels like we sacrifice quite a lot
for appearing faster.

> >> You also have devices that require firmware download to be
> >> functional, or some sort of internal ramp-up needed.
> > 
> > Yeah these can be fairly annoying. But both of these can be
> > tackled with either handling the situation in probe, or if that
> > makes boot too slow, moving to a similar child driver approach.
> 
> Both options are not so good IMHO...

What is it you dislike about them? Just the slower boot, or
something more fundamental?

Thanks,
Charles

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Pierre-Louis Bossart]

On 1/13/26 18:27, Charles Keepax wrote:
> On Tue, Jan 06, 2026 at 06:10:06PM +0100, Pierre-Louis Bossart wrote:
>> On 1/6/26 13:58, Charles Keepax wrote:
>>> On Sat, Dec 20, 2025 at 12:04:47PM +0100, Pierre-Louis Bossart wrote:
>>>> On 12/10/25 10:55, Charles Keepax wrote:
>>>>> On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
>>>> The only issue I have with it is whether one would deregister
>>>> the child device on a soft reset or whenever the device
>>>> loses sync? It'd be somewhat ugly to do so, but then again
>>>> we have the issue that for the second enumeration we already
>>>> have a probed child driver, which would bring us back to an
>>>> async model really. The 'beauty' of the current model is
>>>> that the probe does nothing really, everything happens at
>>>> enumeration/sync loss. If we dynamically add/remove child
>>>> devices it'll be 'fun' really quickly.
>>>
>>> Yeah that is one of the questions I have pondered a few times.
>>> Were I usually get to is separating out expected cases of
>>> something dropping off the bus and unexpected cases. For say
>>> suspend/resume it is quite likely the device will drop off the
>>> bus and that is expected and probably shouldn't result in the
>>> child drivers being removed, as it will as you say become 'fun'
>>> very quickly. However, an unexpected drop off the bus during
>>> normal operation is really a pretty fatal error. In many ways the
>>> best thing to do is probably to remove the child drivers in this
>>> case, since that will tear down the sound card, and allow
>>> everything to be rebuilt. But these are far from fully formed
>>> ideas, for the most part at the moment we just report things went
>>> bad.
>>
>> A device falling off the bus is not that rare in early
>> bring-up or experimental platforms.  IIRC some devices will
>> require a full reset after firmware download.  Not to mention
>> the glitches that can be seen consistently after clock stop
>> on some platforms.
> 
> So doing a reset after firmware download should be fine, I would
> consider than an expected drop off.
> 
>> My take is that it's better to 'hide' some of the low-level
>> detail and keep the card visible to apps, always.
> 
> I would agree here, it is better if the card stays present and
> hides non-error states.

Sorry, I am not following what you meant by 'hide non-error states'

If the card stays present, that would require the child device to remain registered.
Otherwise if the child device is unregistered, the card would be torn down due to missing resources, no?
Or maybe we end-up with a case where the resource is already in-use so the child device cannot be unregistered...

>> That means the drivers need to deal with cases where accesses
>> to devices might be delayed due to slow sync or sync loss. As
>> long as everything recovers we are good to go.
> 
> Yeah that is a point we slightly disagree on, I really am not
> a huge fan of auto-recovery stuff. Been involved in too many
> situations where the auto-recovery recovers *most* of the time
> and takes a 1/10 fail to a 1/10000 fail.
> 
> Taking this case, if the device loses sync during audio that
> is gonna glitch the audio, which makes it a problem I will be
> expected to fix. I would rather get a report the audio stopped
> with a log up to that point, than get reports of audio glitches.

I don't disagree about audio glitches, my main problem is with sync loss during initialization or suspend-resume.

Maybe a tangent but if a device loses sync while audio is playing, there would be no xrun signaled to the upper layers. The bus allocation reserves specific slots for specific streams, and the data will be written/read whether there's a device or not. That's how we tested the 'virtual' devices to create pretend master-only audio cards with no actual codecs attached.
If we really want to report glitches due to sync loss during audio playback, we have some plumbing to do. I tested with a couple of years ago by forcing a device to reset with a debugfs command, it wasn't detected by any ALSA layers.

>>>> That's precisely the problem, the model assumes something
>>>> that is broken left and right in practice.
>>>> Exhibit A is the PCI/HDaudio parts where we rely on an async
>>>> probe due to the module handling.
>>>
>>> I think I am perhaps not totally familiar with the issues on the
>>> PCI/HDaudio side. What is the primary issue here?
>>
>> The HDAudio side loads different modules, which can take a lot
>> of time. For that reason, the driver probe returns immediately
>> but most of the processing is done in a workqueue. This leads
>> to two problems
>> a) if the processing fails for some reason, we have no way
>>    of signaling any error.
>> b) if the card is created independently from the PCI driver,
>>    e.g. with ACPI information, then we have no way of telling
>>    that the resources needed by the card are available.
> 
> I mean sure it does make the boot look faster, but is it really
> faster as the devices weren't actually ready when we told
> user-space they were? And now we have to deal with the problems
> outlined here, as well as what happens if user-space/another
> driver tries to use the device whilst it is in this
> not-quite-ready state. It feels like we sacrifice quite a lot
> for appearing faster.
> 
>>>> You also have devices that require firmware download to be
>>>> functional, or some sort of internal ramp-up needed.
>>>
>>> Yeah these can be fairly annoying. But both of these can be
>>> tackled with either handling the situation in probe, or if that
>>> makes boot too slow, moving to a similar child driver approach.
>>
>> Both options are not so good IMHO...
> 
> What is it you dislike about them? Just the slower boot, or
> something more fundamental?

The slower boot isn't going to be accepted by anyone, and in the specific case of HDaudio I don't think anyone has an appetite for changing the probe/workqueue approach that's been around for many many years.
I don't think creating a child device is much better than using a workqueue, in both cases errors during the two-step probe can't be handled, you end-up with zoombie devices that are probed but not functional.

The only benefit you get with the child device is that if it provides the resources required by the deferred probe you have nothing to do. the open is what happens when those resources are not present due to sync loss.

Re: [PATCH 3/4] ASoC: SDCA: Add basic SDCA class driver

[Charles Keepax]

On Tue, Jan 13, 2026 at 11:05:31PM +0100, Pierre-Louis Bossart wrote:
> On 1/13/26 18:27, Charles Keepax wrote:
> > On Tue, Jan 06, 2026 at 06:10:06PM +0100, Pierre-Louis Bossart wrote:
> >> On 1/6/26 13:58, Charles Keepax wrote:
> >>> On Sat, Dec 20, 2025 at 12:04:47PM +0100, Pierre-Louis Bossart wrote:
> >>>> On 12/10/25 10:55, Charles Keepax wrote:
> >>>>> On Tue, Dec 09, 2025 at 12:47:06PM +0000, Pierre-Louis Bossart wrote:
> >> That means the drivers need to deal with cases where accesses
> >> to devices might be delayed due to slow sync or sync loss. As
> >> long as everything recovers we are good to go.
> > 
> > Yeah that is a point we slightly disagree on, I really am not
> > a huge fan of auto-recovery stuff. Been involved in too many
> > situations where the auto-recovery recovers *most* of the time
> > and takes a 1/10 fail to a 1/10000 fail.
> > 
> > Taking this case, if the device loses sync during audio that
> > is gonna glitch the audio, which makes it a problem I will be
> > expected to fix. I would rather get a report the audio stopped
> > with a log up to that point, than get reports of audio glitches.
> 
> I don't disagree about audio glitches, my main problem is
> with sync loss during initialization or suspend-resume.

I think we are in agreement then, I would consider dropping off
the bus during init or suspend-resume to be times we expect this
to happen and the driver can take care of it.

> Maybe a tangent but if a device loses sync while audio
> is playing, there would be no xrun signaled to the upper
> layers. The bus allocation reserves specific slots for specific
> streams, and the data will be written/read whether there's a
> device or not. That's how we tested the 'virtual' devices to
> create pretend master-only audio cards with no actual codecs
> attached.
> If we really want to report glitches due to sync loss during
> audio playback, we have some plumbing to do. I tested with
> a couple of years ago by forcing a device to reset with a
> debugfs command, it wasn't detected by any ALSA layers.

Slight tangent, my concern is less about where the glitch gets
reported and more that I don't want them to happen.

> >>> Yeah these can be fairly annoying. But both of these can be
> >>> tackled with either handling the situation in probe, or if that
> >>> makes boot too slow, moving to a similar child driver approach.
> >>
> >> Both options are not so good IMHO...
> > 
> > What is it you dislike about them? Just the slower boot, or
> > something more fundamental?
> 
> The slower boot isn't going to be accepted by anyone, and
> in the specific case of HDaudio I don't think anyone has an
> appetite for changing the probe/workqueue approach that's been
> around for many many years.

I mean there is certainly an argument for not mucking around with
HDaudio, it would seem bold at this point. We have been managing with
how things are and its a bus that is coming to the end of its life.

> I don't think creating a child device is much better than
> using a workqueue, in both cases errors during the two-step
> probe can't be handled, you end-up with zoombie devices that
> are probed but not functional.

No more than you end up with a zombie device using a workqueue
system. If the bits in the workqueue don't succeed you have a
device that has probed but is not functional as well. I really
don't see what the difference is there.

As for error propogation seems better with the two devices
setup, everything that is waiting for the child devices knows
they haven't probed yet, so can fail itself.

> The only benefit you get with the child device is that if it
> provides the resources required by the deferred probe you have
> nothing to do. the open is what happens when those resources
> are not present due to sync loss.

I just don't really see why the sync loss thing is different
either. You need to decide in either approach how to handle when
the device loses sync, things depend on functionality provided
regardless of how you structure the driver.

Thanks,
Charles

[PATCH 4/4] ASoC: SDCA: Add basic SDCA function driver

[Charles Keepax]

Add a driver to support the individual SDCA functions within the class
driver.

Co-developed-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/Kconfig               |   7 +
 sound/soc/sdca/Makefile              |   2 +
 sound/soc/sdca/sdca_class_function.c | 459 +++++++++++++++++++++++++++
 3 files changed, 468 insertions(+)
 create mode 100644 sound/soc/sdca/sdca_class_function.c

diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
index 56e3ff8448762..0a37e94a60302 100644
--- a/sound/soc/sdca/Kconfig
+++ b/sound/soc/sdca/Kconfig
@@ -39,6 +39,7 @@ config SND_SOC_SDCA_OPTIONAL
 
 config SND_SOC_SDCA_CLASS
 	tristate "SDCA Class Driver"
+	select SND_SOC_SDCA_CLASS_FUNCTION
 	select SND_SOC_SDCA
 	select SND_SOC_SDCA_FDL
 	select SND_SOC_SDCA_HID
@@ -47,4 +48,10 @@ config SND_SOC_SDCA_CLASS
 	  This option enables support for the SDCA Class driver which should
 	  support any class compliant SDCA part.
 
+config SND_SOC_SDCA_CLASS_FUNCTION
+	tristate
+	help
+	  This option enables support for the SDCA Class Function drivers,
+	  these implement the individual functions of the SDCA Class driver.
+
 endmenu
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index 95db4cef34833..f6b73275d9649 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -7,7 +7,9 @@ snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_IRQ) += sdca_interrupts.o
 snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_FDL) += sdca_fdl.o
 
 snd-soc-sdca-class-y := sdca_class.o
+snd-soc-sdca-class-function-y := sdca_class_function.o
 
 obj-$(CONFIG_SND_SOC_SDCA) += snd-soc-sdca.o
 
 obj-$(CONFIG_SND_SOC_SDCA_CLASS) += snd-soc-sdca-class.o
+obj-$(CONFIG_SND_SOC_SDCA_CLASS_FUNCTION) += snd-soc-sdca-class-function.o
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
new file mode 100644
index 0000000000000..34adedbf2fb5a
--- /dev/null
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Cirrus Logic, Inc. and
+//                    Cirrus Logic International Semiconductor Ltd.
+
+/*
+ * The MIPI SDCA specification is available for public downloads at
+ * https://www.mipi.org/mipi-sdca-v1-0-download
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <sound/pcm.h>
+#include <sound/sdca_asoc.h>
+#include <sound/sdca_fdl.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/sdca_regmap.h>
+#include <sound/sdw.h>
+#include <sound/soc-component.h>
+#include <sound/soc-dai.h>
+#include <sound/soc.h>
+#include "sdca_class.h"
+
+struct class_function_drv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct sdca_class_drv *core;
+
+	struct sdca_function_data *function;
+};
+
+static void class_function_regmap_lock(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_lock(lock);
+}
+
+static void class_function_regmap_unlock(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_unlock(lock);
+}
+
+static bool class_function_regmap_writeable(struct device *dev, unsigned int reg)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	return sdca_regmap_writeable(drv->function, reg);
+}
+
+static bool class_function_regmap_readable(struct device *dev, unsigned int reg)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	return sdca_regmap_readable(drv->function, reg);
+}
+
+static bool class_function_regmap_volatile(struct device *dev, unsigned int reg)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	return sdca_regmap_volatile(drv->function, reg);
+}
+
+static const struct regmap_config class_function_regmap_config = {
+	.name			= "sdca",
+	.reg_bits		= 32,
+	.val_bits		= 32,
+	.reg_format_endian	= REGMAP_ENDIAN_LITTLE,
+	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
+
+	.max_register		= SDW_SDCA_MAX_REGISTER,
+	.readable_reg		= class_function_regmap_readable,
+	.writeable_reg		= class_function_regmap_writeable,
+	.volatile_reg		= class_function_regmap_volatile,
+
+	.cache_type		= REGCACHE_MAPLE,
+
+	.lock			= class_function_regmap_lock,
+	.unlock			= class_function_regmap_unlock,
+};
+
+static int class_function_regmap_mbq_size(struct device *dev, unsigned int reg)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	return sdca_regmap_mbq_size(drv->function, reg);
+}
+
+static bool class_function_regmap_deferrable(struct device *dev, unsigned int reg)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	return sdca_regmap_deferrable(drv->function, reg);
+}
+
+static const struct regmap_sdw_mbq_cfg class_function_mbq_config = {
+	.mbq_size		= class_function_regmap_mbq_size,
+	.deferrable		= class_function_regmap_deferrable,
+	.retry_us		= 1000,
+	.timeout_us		= 10000,
+};
+
+static int class_function_startup(struct snd_pcm_substream *substream,
+				  struct snd_soc_dai *dai)
+{
+	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
+
+	return sdca_asoc_set_constraints(drv->dev, drv->regmap, drv->function,
+					 substream, dai);
+}
+
+static int class_function_sdw_add_peripheral(struct snd_pcm_substream *substream,
+					     struct snd_pcm_hw_params *params,
+					     struct snd_soc_dai *dai)
+{
+	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
+	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
+	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
+	struct sdw_stream_config sconfig = {0};
+	struct sdw_port_config pconfig = {0};
+	int ret;
+
+	if (!sdw_stream)
+		return -EINVAL;
+
+	snd_sdw_params_to_config(substream, params, &sconfig, &pconfig);
+
+	ret = sdca_asoc_get_port(drv->dev, drv->regmap, drv->function, dai);
+	if (ret < 0)
+		return ret;
+
+	pconfig.num = ret;
+
+	ret = sdw_stream_add_slave(sdw, &sconfig, &pconfig, 1, sdw_stream);
+	if (ret) {
+		dev_err(drv->dev, "failed to add sdw stream: %d\n", ret);
+		return ret;
+	}
+
+	return sdca_asoc_hw_params(drv->dev, drv->regmap, drv->function,
+				   substream, params, dai);
+}
+
+static int class_function_sdw_remove_peripheral(struct snd_pcm_substream *substream,
+						struct snd_soc_dai *dai)
+{
+	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
+	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
+	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
+
+	if (!sdw_stream)
+		return -EINVAL;
+
+	return sdw_stream_remove_slave(sdw, sdw_stream);
+}
+
+static int class_function_sdw_set_stream(struct snd_soc_dai *dai, void *sdw_stream,
+					 int direction)
+{
+	snd_soc_dai_dma_data_set(dai, direction, sdw_stream);
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops class_function_sdw_ops = {
+	.startup	= class_function_startup,
+	.shutdown	= sdca_asoc_free_constraints,
+	.set_stream	= class_function_sdw_set_stream,
+	.hw_params	= class_function_sdw_add_peripheral,
+	.hw_free	= class_function_sdw_remove_peripheral,
+};
+
+static int class_function_component_probe(struct snd_soc_component *component)
+{
+	struct class_function_drv *drv = snd_soc_component_get_drvdata(component);
+	struct sdca_class_drv *core = drv->core;
+
+	return sdca_irq_populate(drv->function, component, core->irq_info);
+}
+
+static const struct snd_soc_component_driver class_function_component_drv = {
+	.probe			= class_function_component_probe,
+	.endianness		= 1,
+};
+
+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;
+	}
+
+	if (!(val & 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);
+		if (ret)
+			return ret;
+	}
+
+	if (val & 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;
+		}
+	}
+
+	/* Start FDL process */
+	ret = sdca_irq_populate_early(drv->dev, drv->regmap, drv->function,
+				      drv->core->irq_info);
+	if (ret)
+		return ret;
+
+	ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
+	if (ret)
+		return ret;
+
+	ret = sdca_regmap_write_defaults(drv->dev, drv->regmap, drv->function);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(drv->regmap, reg, 0xFF);
+	if (ret < 0) {
+		dev_err(drv->dev, "failed to clear function status: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int class_function_probe(struct auxiliary_device *auxdev,
+				const struct auxiliary_device_id *aux_dev_id)
+{
+	struct device *dev = &auxdev->dev;
+	struct sdca_class_drv *core = dev_get_drvdata(dev->parent);
+	struct sdca_device_data *data = &core->sdw->sdca_data;
+	struct sdca_function_desc *desc;
+	struct snd_soc_component_driver *cmp_drv;
+	struct snd_soc_dai_driver *dais;
+	struct class_function_drv *drv;
+	struct regmap_sdw_mbq_cfg *mbq_config;
+	struct regmap_config *config;
+	struct reg_default *defaults;
+	int ndefaults;
+	int num_dais;
+	int ret;
+	int i;
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	cmp_drv = devm_kmemdup(dev, &class_function_component_drv, sizeof(*cmp_drv),
+			       GFP_KERNEL);
+	if (!cmp_drv)
+		return -ENOMEM;
+
+	config = devm_kmemdup(dev, &class_function_regmap_config, sizeof(*config),
+			      GFP_KERNEL);
+	if (!config)
+		return -ENOMEM;
+
+	mbq_config = devm_kmemdup(dev, &class_function_mbq_config, sizeof(*mbq_config),
+				  GFP_KERNEL);
+	if (!mbq_config)
+		return -ENOMEM;
+
+	drv->dev = dev;
+	drv->core = core;
+
+	for (i = 0; i < data->num_functions; i++) {
+		desc = &data->function[i];
+
+		if (desc->type == aux_dev_id->driver_data)
+			break;
+	}
+	if (i == core->sdw->sdca_data.num_functions) {
+		dev_err(dev, "failed to locate function\n");
+		return -EINVAL;
+	}
+
+	drv->function = &core->functions[i];
+
+	ret = sdca_parse_function(dev, core->sdw, desc, drv->function);
+	if (ret)
+		return ret;
+
+	ndefaults = sdca_regmap_count_constants(dev, drv->function);
+	if (ndefaults < 0)
+		return ndefaults;
+
+	defaults = devm_kcalloc(dev, ndefaults, sizeof(*defaults), GFP_KERNEL);
+	if (!defaults)
+		return -ENOMEM;
+
+	ret = sdca_regmap_populate_constants(dev, drv->function, defaults);
+	if (ret < 0)
+		return ret;
+
+	regcache_sort_defaults(defaults, ndefaults);
+
+	auxiliary_set_drvdata(auxdev, drv);
+
+	config->reg_defaults = defaults;
+	config->num_reg_defaults = ndefaults;
+	config->lock_arg = &core->regmap_lock;
+
+	if (drv->function->busy_max_delay) {
+		mbq_config->timeout_us = drv->function->busy_max_delay;
+		mbq_config->retry_us = umax(drv->function->busy_max_delay / 10,
+					    mbq_config->retry_us);
+	}
+
+	drv->regmap = devm_regmap_init_sdw_mbq_cfg(dev, core->sdw, config, mbq_config);
+	if (IS_ERR(drv->regmap))
+		return dev_err_probe(dev, PTR_ERR(drv->regmap),
+				     "failed to create regmap");
+
+	ret = sdca_asoc_populate_component(dev, drv->function, cmp_drv,
+					   &dais, &num_dais,
+					   &class_function_sdw_ops);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(dev, 200);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	ret = class_function_boot(drv);
+	if (ret)
+		return ret;
+
+	ret = devm_snd_soc_register_component(dev, cmp_drv, dais, num_dais);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register component\n");
+
+	dev_err(dev, "%s: %pfwP: probe completed\n", __func__, auxdev->dev.fwnode);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+static int class_function_codec_runtime_suspend(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+
+	/*
+	 * Whilst the driver doesn't power the chip down here, going into
+	 * runtime suspend lets the SoundWire bus power down, which means
+	 * the driver can't communicate with the device any more.
+	 */
+	regcache_cache_only(drv->regmap, true);
+
+	return 0;
+}
+
+static int class_function_codec_runtime_resume(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+	int ret;
+
+	regcache_cache_only(drv->regmap, false);
+
+	regcache_mark_dirty(drv->regmap);
+
+	ret = regcache_sync(drv->regmap);
+	if (ret) {
+		dev_err(drv->dev, "failed to restore register cache: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	regcache_cache_only(drv->regmap, true);
+
+	return ret;
+}
+
+static const struct dev_pm_ops class_function_pm_ops = {
+	RUNTIME_PM_OPS(class_function_codec_runtime_suspend,
+		       class_function_codec_runtime_resume, NULL)
+};
+
+static const struct auxiliary_device_id class_function_id_table[] = {
+	{
+		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_AMP_NAME,
+		.driver_data = SDCA_FUNCTION_TYPE_SMART_AMP,
+	},
+	{
+		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_MIC_NAME,
+		.driver_data = SDCA_FUNCTION_TYPE_SMART_MIC,
+	},
+	{
+		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_UAJ_NAME,
+		.driver_data = SDCA_FUNCTION_TYPE_UAJ,
+	},
+	{
+		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_HID_NAME,
+		.driver_data = SDCA_FUNCTION_TYPE_HID,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(auxiliary, class_function_id_table);
+
+static struct auxiliary_driver class_function_drv = {
+	.driver = {
+		.name		= "sdca_function",
+		.pm		= pm_ptr(&class_function_pm_ops),
+	},
+
+	.probe		= class_function_probe,
+	.id_table	= class_function_id_table
+};
+module_auxiliary_driver(class_function_drv);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SDCA Class Function Driver");
+MODULE_IMPORT_NS("SND_SOC_SDCA");
-- 
2.47.3

Re: [PATCH 4/4] ASoC: SDCA: Add basic SDCA function driver

[Pierre-Louis Bossart]

> +static int class_function_sdw_add_peripheral(struct snd_pcm_substream *substream,
> +					     struct snd_pcm_hw_params *params,
> +					     struct snd_soc_dai *dai)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
> +	struct sdw_stream_config sconfig = {0};
> +	struct sdw_port_config pconfig = {0};
> +	int ret;
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	snd_sdw_params_to_config(substream, params, &sconfig, &pconfig);
> +
> +	ret = sdca_asoc_get_port(drv->dev, drv->regmap, drv->function, dai);
> +	if (ret < 0)
> +		return ret;
> +
> +	pconfig.num = ret;

You should probably document the assumption that each function has access to separate ports. 

IIRC there could be cases where ports are *shared* between functions due to hardware restrictions. I remember telling the SDCA committee that either functions were independent or they were not, and the answer is that they are independent except when they aren't. It's probably ok to assume a simple case first to make progress, but with a clear documentation of the known limits.

Oh and for some functions multiple ports are required to make use of different lanes, e.g. for the digital links in the NDAI functions.

> +
> +	ret = sdw_stream_add_slave(sdw, &sconfig, &pconfig, 1, sdw_stream);
> +	if (ret) {
> +		dev_err(drv->dev, "failed to add sdw stream: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return sdca_asoc_hw_params(drv->dev, drv->regmap, drv->function,
> +				   substream, params, dai);
> +}
> +
> +static int class_function_sdw_remove_peripheral(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	return sdw_stream_remove_slave(sdw, sdw_stream);
> +}
> +
> +static int class_function_sdw_set_stream(struct snd_soc_dai *dai, void *sdw_stream,
> +					 int direction)
> +{
> +	snd_soc_dai_dma_data_set(dai, direction, sdw_stream);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops class_function_sdw_ops = {
> +	.startup	= class_function_startup,
> +	.shutdown	= sdca_asoc_free_constraints,
> +	.set_stream	= class_function_sdw_set_stream,
> +	.hw_params	= class_function_sdw_add_peripheral,
> +	.hw_free	= class_function_sdw_remove_peripheral,
> +};
> +
> +static int class_function_component_probe(struct snd_soc_component *component)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(component);
> +	struct sdca_class_drv *core = drv->core;
> +
> +	return sdca_irq_populate(drv->function, component, core->irq_info);
> +}
> +
> +static const struct snd_soc_component_driver class_function_component_drv = {
> +	.probe			= class_function_component_probe,
> +	.endianness		= 1,
> +};
> +
> +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;
> +	}
> +
> +	if (!(val & 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);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (val & 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;
> +		}
> +	}
> +
> +	/* Start FDL process */
> +	ret = sdca_irq_populate_early(drv->dev, drv->regmap, drv->function,
> +				      drv->core->irq_info);
> +	if (ret)
> +		return ret;
> +
> +	ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
> +	if (ret)
> +		return ret;
> +
> +	ret = sdca_regmap_write_defaults(drv->dev, drv->regmap, drv->function);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(drv->regmap, reg, 0xFF);
> +	if (ret < 0) {
> +		dev_err(drv->dev, "failed to clear function status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int class_function_probe(struct auxiliary_device *auxdev,
> +				const struct auxiliary_device_id *aux_dev_id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct sdca_class_drv *core = dev_get_drvdata(dev->parent);
> +	struct sdca_device_data *data = &core->sdw->sdca_data;
> +	struct sdca_function_desc *desc;
> +	struct snd_soc_component_driver *cmp_drv;
> +	struct snd_soc_dai_driver *dais;
> +	struct class_function_drv *drv;
> +	struct regmap_sdw_mbq_cfg *mbq_config;
> +	struct regmap_config *config;
> +	struct reg_default *defaults;
> +	int ndefaults;
> +	int num_dais;
> +	int ret;
> +	int i;
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	cmp_drv = devm_kmemdup(dev, &class_function_component_drv, sizeof(*cmp_drv),
> +			       GFP_KERNEL);
> +	if (!cmp_drv)
> +		return -ENOMEM;
> +
> +	config = devm_kmemdup(dev, &class_function_regmap_config, sizeof(*config),
> +			      GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
> +
> +	mbq_config = devm_kmemdup(dev, &class_function_mbq_config, sizeof(*mbq_config),
> +				  GFP_KERNEL);
> +	if (!mbq_config)
> +		return -ENOMEM;
> +
> +	drv->dev = dev;
> +	drv->core = core;
> +
> +	for (i = 0; i < data->num_functions; i++) {
> +		desc = &data->function[i];
> +
> +		if (desc->type == aux_dev_id->driver_data)
> +			break;
> +	}
> +	if (i == core->sdw->sdca_data.num_functions) {
> +		dev_err(dev, "failed to locate function\n");
> +		return -EINVAL;
> +	}
> +
> +	drv->function = &core->functions[i];
> +
> +	ret = sdca_parse_function(dev, core->sdw, desc, drv->function);
> +	if (ret)
> +		return ret;
> +
> +	ndefaults = sdca_regmap_count_constants(dev, drv->function);
> +	if (ndefaults < 0)
> +		return ndefaults;
> +
> +	defaults = devm_kcalloc(dev, ndefaults, sizeof(*defaults), GFP_KERNEL);
> +	if (!defaults)
> +		return -ENOMEM;
> +
> +	ret = sdca_regmap_populate_constants(dev, drv->function, defaults);
> +	if (ret < 0)
> +		return ret;
> +
> +	regcache_sort_defaults(defaults, ndefaults);
> +
> +	auxiliary_set_drvdata(auxdev, drv);
> +
> +	config->reg_defaults = defaults;
> +	config->num_reg_defaults = ndefaults;
> +	config->lock_arg = &core->regmap_lock;
> +
> +	if (drv->function->busy_max_delay) {
> +		mbq_config->timeout_us = drv->function->busy_max_delay;
> +		mbq_config->retry_us = umax(drv->function->busy_max_delay / 10,
> +					    mbq_config->retry_us);
> +	}
> +
> +	drv->regmap = devm_regmap_init_sdw_mbq_cfg(dev, core->sdw, config, mbq_config);
> +	if (IS_ERR(drv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(drv->regmap),
> +				     "failed to create regmap");
> +
> +	ret = sdca_asoc_populate_component(dev, drv->function, cmp_drv,
> +					   &dais, &num_dais,
> +					   &class_function_sdw_ops);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 200);

why 200?

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
same comment, this is the same sequence as for the parent and I don't quite follow the pm_runtime flow.

> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = class_function_boot(drv);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_snd_soc_register_component(dev, cmp_drv, dais, num_dais);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register component\n");
> +
> +	dev_err(dev, "%s: %pfwP: probe completed\n", __func__, auxdev->dev.fwnode);

why is this an error? the error case is handled earlier.

> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +}
> +
> +static int class_function_codec_runtime_suspend(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> +
> +	/*
> +	 * Whilst the driver doesn't power the chip down here, going into
> +	 * runtime suspend lets the SoundWire bus power down, which means
> +	 * the driver can't communicate with the device any more.
> +	 */

actually this is not completely correct. The function driver can communicate IF the parent is active. This is copy-paste from the device level but it's not how the hardware works.

> +	regcache_cache_only(drv->regmap, true);

I would also add a comment that all the power management for PE/GEs is handled by DAPM when streams become active/suspended. So there isn't a need to deal with PE/GEs at this level.

> +
> +	return 0;
> +}
> +
> +static int class_function_codec_runtime_resume(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> +	int ret;
> +
> +	regcache_cache_only(drv->regmap, false);
> +
> +	regcache_mark_dirty(drv->regmap);

does the order matter?

> +
> +	ret = regcache_sync(drv->regmap);
> +	if (ret) {
> +		dev_err(drv->dev, "failed to restore register cache: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	regcache_cache_only(drv->regmap, true);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops class_function_pm_ops = {
> +	RUNTIME_PM_OPS(class_function_codec_runtime_suspend,
> +		       class_function_codec_runtime_resume, NULL)
> +};
> +
> +static const struct auxiliary_device_id class_function_id_table[] = {
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_AMP_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_SMART_AMP,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_MIC_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_SMART_MIC,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_UAJ_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_UAJ,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_HID_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_HID,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, class_function_id_table);
> +
> +static struct auxiliary_driver class_function_drv = {
> +	.driver = {
> +		.name		= "sdca_function",
> +		.pm		= pm_ptr(&class_function_pm_ops),
> +	},
> +
> +	.probe		= class_function_probe,
> +	.id_table	= class_function_id_table
> +};
> +module_auxiliary_driver(class_function_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SDCA Class Function Driver");
> +MODULE_IMPORT_NS("SND_SOC_SDCA");

Re: [PATCH 4/4] ASoC: SDCA: Add basic SDCA function driver

[Charles Keepax]

On Mon, Oct 27, 2025 at 04:24:05PM +0100, Pierre-Louis Bossart wrote:
> > +	snd_sdw_params_to_config(substream, params, &sconfig, &pconfig);
> > +
> > +	ret = sdca_asoc_get_port(drv->dev, drv->regmap, drv->function, dai);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pconfig.num = ret;
> 
> You should probably document the assumption that each function
> has access to separate ports.
> 
> IIRC there could be cases where ports are *shared* between
> functions due to hardware restrictions. I remember telling the
> SDCA committee that either functions were independent or they were
> not, and the answer is that they are independent except when they
> aren't. It's probably ok to assume a simple case first to make
> progress, but with a clear documentation of the known limits.
> 
> Oh and for some functions multiple ports are required to make use
> of different lanes, e.g. for the digital links in the NDAI functions.

Yeah I will add a comment to clarify we are currently only
supporting a single port, although it does somewhat duplicate the
FIXME already in sdca_asoc_get_port for that.

But overal I think these are definitely bridges we should cross
when we get there. It is more important to get something that
functions rather than it supporting every single feature in the
first iteration (especially given how close we now our to laptops
shipping with parts we were hoping to support through this class
driver).

> > +	pm_runtime_set_autosuspend_delay(dev, 200);
> 
> why 200?

That is a totally made up figure specially selected to be
slightly shorted that the totally made up figure I picked for the
primary driver. Happy to take feedback on these lengths or add a
comment to say they are arbitary although these tend to be
arbitary on all drivers so I didn't feel the need to add the
comment.

> > +	ret = devm_snd_soc_register_component(dev, cmp_drv, dais, num_dais);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register component\n");
> > +
> > +	dev_err(dev, "%s: %pfwP: probe completed\n", __func__, auxdev->dev.fwnode);
> 
> why is this an error? the error case is handled earlier.

Yeah that is just debug that hasn't been nuked yet the whole
print should be deleted.

> > +static int class_function_codec_runtime_suspend(struct device *dev)
> > +{
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> > +
> > +	/*
> > +	 * Whilst the driver doesn't power the chip down here, going into
> > +	 * runtime suspend lets the SoundWire bus power down, which means
> > +	 * the driver can't communicate with the device any more.
> > +	 */
> 
> actually this is not completely correct. The function driver can
> communicate IF the parent is active. This is copy-paste from the
> device level but it's not how the hardware works.

I mean it does say "lets the SoundWire bus power down" not forces
the bus to power down. But I will try to reword to make it more
clear this is covering the fact the bus might power down rather
than will power down.

> > +	regcache_cache_only(drv->regmap, true);
> 
> I would also add a comment that all the power management for
> PE/GEs is handled by DAPM when streams become active/suspended. So
> there isn't a need to deal with PE/GEs at this level.

Can do.

> > +static int class_function_codec_runtime_resume(struct device *dev)
> > +{
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> > +	int ret;
> > +
> > +	regcache_cache_only(drv->regmap, false);
> > +
> > +	regcache_mark_dirty(drv->regmap);
> 
> does the order matter?

I don't believe it does, happy to reverse them.

Thanks,
Charles