[PATCH 00/15] Add SDCA UMP/FDL support

[PATCH 00/15] Add SDCA UMP/FDL support

[Charles Keepax]

Next installment of the SDCA changes, hopefully the next series after
this should be the full class driver. It is worth noting this series has
a build dependency on a patch working its way through the PM/ACPI tree:

ac46f5b6c661 ("ACPICA: Add SoundWire File Table (SWFT) signature")
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

But we can probably worry about that later, as normally there is a
reasonable amount of review on these SDCA series'.

This series broadly breaks down into 3 chunks, first there are several
changes to remove the assumption that the struct device used for SDCA
purposes represents the SoundWire slave. This is because the SDCA class
driver will be made of an auxiliary driver for each SDCA Function, thus
the SoundWire slave will be on the parent device for each individual
driver. Then there are patches to add support for UMP/FDL. And then
finally since the rest of the HID support is there and UMP was the last
missing part required a small patch to add a function to allow reporting
of HID events from SDCA devices.

Thanks,
Charles

Charles Keepax (12):
  regmap: sdw-mbq: Don't assume the regmap device is the SoundWire slave
  ASoC: SDCA: Add manual PM runtime gets to IRQ handlers
  ASoC: SDCA: Pass SoundWire slave to HID
  ASoC: SDCA: Pass device register map from IRQ alloc to handlers
  ASoC: SDCA: Update externally_requested flag to cover all requests
  ASoC: SDCA: Factor out a helper to find SDCA IRQ data
  ASoC: SDCA: Rely less on the ASoC component in IRQ handling
  ASoC: SDCA: Force some SDCA Controls to be volatile
  ASoC: SDCA: Add UMP buffer helper functions
  ASoC: SDCA: Add completion for FDL start and stop
  ASoC: SDCA: Add early IRQ handling
  ASoC: SDCA: Add HID button IRQ

Maciej Strozek (3):
  ASoC: SDCA: Add SDCA FDL data parsing
  ASoC: SDCA: Add FDL library for XU entities
  ASoC: SDCA: Add XU-specific IRQ processing

 drivers/base/regmap/regmap-sdw-mbq.c |  23 +-
 include/linux/regmap.h               |  21 +-
 include/sound/sdca.h                 |   5 +
 include/sound/sdca_entity_xu.h       |  63 +++++
 include/sound/sdca_function.h        |  90 ++++++-
 include/sound/sdca_hid.h             |  21 +-
 include/sound/sdca_interrupts.h      |  19 +-
 include/sound/sdca_ump.h             |  45 ++++
 sound/soc/codecs/rt722-sdca-sdw.c    |   4 +-
 sound/soc/sdca/Kconfig               |   7 +
 sound/soc/sdca/Makefile              |   4 +-
 sound/soc/sdca/sdca_device.c         |  20 ++
 sound/soc/sdca/sdca_entity_xu.c      | 384 +++++++++++++++++++++++++++
 sound/soc/sdca/sdca_functions.c      | 171 +++++++++++-
 sound/soc/sdca/sdca_hid.c            |  58 +++-
 sound/soc/sdca/sdca_interrupts.c     | 257 +++++++++++++++---
 sound/soc/sdca/sdca_regmap.c         |   9 +-
 sound/soc/sdca/sdca_ump.c            | 247 +++++++++++++++++
 18 files changed, 1353 insertions(+), 95 deletions(-)
 create mode 100644 include/sound/sdca_entity_xu.h
 create mode 100644 include/sound/sdca_ump.h
 create mode 100644 sound/soc/sdca/sdca_entity_xu.c
 create mode 100644 sound/soc/sdca/sdca_ump.c

-- 
2.47.2

[PATCH 01/15] regmap: sdw-mbq: Don't assume the regmap device is the SoundWire slave

[Charles Keepax]

Currently, the code assumes that the device that registered the
MBQ register map is the actual SoundWire slave device. This works
fine for all current users, however future SDCA devices will
likely be implemented with the SoundWire slave as a parent device
and separate child drivers with regmaps for each audio Function.
Update the regmap_init_sdw_mbq_cfg macro to allow these two
to be specified separately.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/base/regmap/regmap-sdw-mbq.c | 23 ++++++++++++-----------
 include/linux/regmap.h               | 21 +++++++++++----------
 sound/soc/codecs/rt722-sdca-sdw.c    |  4 +++-
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/base/regmap/regmap-sdw-mbq.c b/drivers/base/regmap/regmap-sdw-mbq.c
index 86644bbd07100..8b7d34a6080d2 100644
--- a/drivers/base/regmap/regmap-sdw-mbq.c
+++ b/drivers/base/regmap/regmap-sdw-mbq.c
@@ -15,6 +15,7 @@
 
 struct regmap_mbq_context {
 	struct device *dev;
+	struct sdw_slave *sdw;
 
 	struct regmap_sdw_mbq_cfg cfg;
 
@@ -46,7 +47,7 @@ static bool regmap_sdw_mbq_deferrable(struct regmap_mbq_context *ctx, unsigned i
 static int regmap_sdw_mbq_poll_busy(struct sdw_slave *slave, unsigned int reg,
 				    struct regmap_mbq_context *ctx)
 {
-	struct device *dev = &slave->dev;
+	struct device *dev = ctx->dev;
 	int val, ret = 0;
 
 	dev_dbg(dev, "Deferring transaction for 0x%x\n", reg);
@@ -96,8 +97,7 @@ static int regmap_sdw_mbq_write_impl(struct sdw_slave *slave,
 static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct regmap_mbq_context *ctx = context;
-	struct device *dev = ctx->dev;
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = ctx->sdw;
 	bool deferrable = regmap_sdw_mbq_deferrable(ctx, reg);
 	int mbq_size = regmap_sdw_mbq_size(ctx, reg);
 	int ret;
@@ -156,8 +156,7 @@ static int regmap_sdw_mbq_read_impl(struct sdw_slave *slave,
 static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct regmap_mbq_context *ctx = context;
-	struct device *dev = ctx->dev;
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+	struct sdw_slave *slave = ctx->sdw;
 	bool deferrable = regmap_sdw_mbq_deferrable(ctx, reg);
 	int mbq_size = regmap_sdw_mbq_size(ctx, reg);
 	int ret;
@@ -208,6 +207,7 @@ static int regmap_sdw_mbq_config_check(const struct regmap_config *config)
 
 static struct regmap_mbq_context *
 regmap_sdw_mbq_gen_context(struct device *dev,
+			   struct sdw_slave *sdw,
 			   const struct regmap_config *config,
 			   const struct regmap_sdw_mbq_cfg *mbq_config)
 {
@@ -218,6 +218,7 @@ regmap_sdw_mbq_gen_context(struct device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	ctx->dev = dev;
+	ctx->sdw = sdw;
 
 	if (mbq_config)
 		ctx->cfg = *mbq_config;
@@ -228,7 +229,7 @@ regmap_sdw_mbq_gen_context(struct device *dev,
 	return ctx;
 }
 
-struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,
+struct regmap *__regmap_init_sdw_mbq(struct device *dev, struct sdw_slave *sdw,
 				     const struct regmap_config *config,
 				     const struct regmap_sdw_mbq_cfg *mbq_config,
 				     struct lock_class_key *lock_key,
@@ -241,16 +242,16 @@ struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,
 	if (ret)
 		return ERR_PTR(ret);
 
-	ctx = regmap_sdw_mbq_gen_context(&sdw->dev, config, mbq_config);
+	ctx = regmap_sdw_mbq_gen_context(dev, sdw, config, mbq_config);
 	if (IS_ERR(ctx))
 		return ERR_CAST(ctx);
 
-	return __regmap_init(&sdw->dev, &regmap_sdw_mbq, ctx,
+	return __regmap_init(dev, &regmap_sdw_mbq, ctx,
 			     config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__regmap_init_sdw_mbq);
 
-struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,
+struct regmap *__devm_regmap_init_sdw_mbq(struct device *dev, struct sdw_slave *sdw,
 					  const struct regmap_config *config,
 					  const struct regmap_sdw_mbq_cfg *mbq_config,
 					  struct lock_class_key *lock_key,
@@ -263,11 +264,11 @@ struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,
 	if (ret)
 		return ERR_PTR(ret);
 
-	ctx = regmap_sdw_mbq_gen_context(&sdw->dev, config, mbq_config);
+	ctx = regmap_sdw_mbq_gen_context(dev, sdw, config, mbq_config);
 	if (IS_ERR(ctx))
 		return ERR_CAST(ctx);
 
-	return __devm_regmap_init(&sdw->dev, &regmap_sdw_mbq, ctx,
+	return __devm_regmap_init(dev, &regmap_sdw_mbq, ctx,
 				  config, lock_key, lock_name);
 }
 EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 4e1ac1fbcec43..70daec535976d 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -676,7 +676,7 @@ struct regmap *__regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
-struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,
+struct regmap *__regmap_init_sdw_mbq(struct device *dev, struct sdw_slave *sdw,
 				     const struct regmap_config *config,
 				     const struct regmap_sdw_mbq_cfg *mbq_config,
 				     struct lock_class_key *lock_key,
@@ -738,7 +738,7 @@ struct regmap *__devm_regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
-struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,
+struct regmap *__devm_regmap_init_sdw_mbq(struct device *dev, struct sdw_slave *sdw,
 					  const struct regmap_config *config,
 					  const struct regmap_sdw_mbq_cfg *mbq_config,
 					  struct lock_class_key *lock_key,
@@ -970,7 +970,7 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
  */
 #define regmap_init_sdw_mbq(sdw, config)					\
 	__regmap_lockdep_wrapper(__regmap_init_sdw_mbq, #config,		\
-				sdw, config, NULL)
+				&sdw->dev, sdw, config, NULL)
 
 /**
  * regmap_init_sdw_mbq_cfg() - Initialise MBQ SDW register map with config
@@ -983,9 +983,9 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
  * to a struct regmap. The regmap will be automatically freed by the
  * device management code.
  */
-#define regmap_init_sdw_mbq_cfg(sdw, config, mbq_config)		\
+#define regmap_init_sdw_mbq_cfg(dev, sdw, config, mbq_config)		\
 	__regmap_lockdep_wrapper(__regmap_init_sdw_mbq, #config,	\
-				sdw, config, mbq_config)
+				dev, sdw, config, mbq_config)
 
 /**
  * regmap_init_spi_avmm() - Initialize register map for Intel SPI Slave
@@ -1198,12 +1198,13 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
  */
 #define devm_regmap_init_sdw_mbq(sdw, config)			\
 	__regmap_lockdep_wrapper(__devm_regmap_init_sdw_mbq, #config,   \
-				sdw, config, NULL)
+				&sdw->dev, sdw, config, NULL)
 
 /**
  * devm_regmap_init_sdw_mbq_cfg() - Initialise managed MBQ SDW register map with config
  *
- * @sdw: Device that will be interacted with
+ * @dev: Device that will be interacted with
+ * @sdw: SoundWire Device that will be interacted with
  * @config: Configuration for register map
  * @mbq_config: Properties for the MBQ registers
  *
@@ -1211,9 +1212,9 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
  * to a struct regmap. The regmap will be automatically freed by the
  * device management code.
  */
-#define devm_regmap_init_sdw_mbq_cfg(sdw, config, mbq_config)	\
-	__regmap_lockdep_wrapper(__devm_regmap_init_sdw_mbq,	\
-				#config, sdw, config, mbq_config)
+#define devm_regmap_init_sdw_mbq_cfg(dev, sdw, config, mbq_config)	\
+	__regmap_lockdep_wrapper(__devm_regmap_init_sdw_mbq,		\
+				#config, dev, sdw, config, mbq_config)
 
 /**
  * devm_regmap_init_slimbus() - Initialise managed register map
diff --git a/sound/soc/codecs/rt722-sdca-sdw.c b/sound/soc/codecs/rt722-sdca-sdw.c
index 70700bdb80a14..e92988aea3f0e 100644
--- a/sound/soc/codecs/rt722-sdca-sdw.c
+++ b/sound/soc/codecs/rt722-sdca-sdw.c
@@ -419,7 +419,9 @@ static int rt722_sdca_sdw_probe(struct sdw_slave *slave,
 	struct regmap *regmap;
 
 	/* Regmap Initialization */
-	regmap = devm_regmap_init_sdw_mbq_cfg(slave, &rt722_sdca_regmap, &rt722_mbq_config);
+	regmap = devm_regmap_init_sdw_mbq_cfg(&slave->dev, slave,
+					      &rt722_sdca_regmap,
+					      &rt722_mbq_config);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
-- 
2.47.2

[PATCH 02/15] ASoC: SDCA: Add manual PM runtime gets to IRQ handlers

[Charles Keepax]

SDCA interrupt handling is a bit odd, the SDCA IRQ registers are
defined on a device level but the handling of the IRQ is at the
Function level. As such the regmap IRQ's PM runtime operations need to
be on the device itself to ensure those registers are available but an
additional runtime get is required for the Function child when the IRQ
is actually handled. Add the required manual PM runtime gets.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_interrupts.c | 42 ++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 79bf3042f57d4..6775f0f7b5659 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -11,7 +11,9 @@
 #include <linux/bits.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
+#include <linux/dev_printk.h>
 #include <linux/interrupt.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
@@ -86,18 +88,25 @@ static irqreturn_t function_status_handler(int irq, void *data)
 {
 	struct sdca_interrupt *interrupt = data;
 	struct device *dev = interrupt->component->dev;
+	irqreturn_t irqret = IRQ_NONE;
 	unsigned int reg, val;
 	unsigned long status;
 	unsigned int mask;
 	int ret;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to resume for function status: %d\n", ret);
+		goto error;
+	}
+
 	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
 			   interrupt->control->sel, 0);
 
 	ret = regmap_read(interrupt->component->regmap, reg, &val);
 	if (ret < 0) {
 		dev_err(dev, "failed to read function status: %d\n", ret);
-		return IRQ_NONE;
+		goto error;
 	}
 
 	dev_dbg(dev, "function status: %#x\n", val);
@@ -130,10 +139,13 @@ static irqreturn_t function_status_handler(int irq, void *data)
 	ret = regmap_write(interrupt->component->regmap, reg, val);
 	if (ret < 0) {
 		dev_err(dev, "failed to clear function status: %d\n", ret);
-		return IRQ_NONE;
+		goto error;
 	}
 
-	return IRQ_HANDLED;
+	irqret = IRQ_HANDLED;
+error:
+	pm_runtime_put(dev);
+	return irqret;
 }
 
 static irqreturn_t detected_mode_handler(int irq, void *data)
@@ -146,21 +158,28 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	struct snd_kcontrol *kctl = interrupt->priv;
 	struct snd_ctl_elem_value *ucontrol __free(kfree) = NULL;
 	struct soc_enum *soc_enum;
+	irqreturn_t irqret = IRQ_NONE;
 	unsigned int reg, val;
 	int ret;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to resume for detected mode: %d\n", ret);
+		goto error;
+	}
+
 	if (!kctl) {
 		const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s %s",
 							   interrupt->entity->label,
 							   SDCA_CTL_SELECTED_MODE_NAME);
 
 		if (!name)
-			return IRQ_NONE;
+			goto error;
 
 		kctl = snd_soc_component_get_kcontrol(component, name);
 		if (!kctl) {
 			dev_dbg(dev, "control not found: %s\n", name);
-			return IRQ_NONE;
+			goto error;
 		}
 
 		interrupt->priv = kctl;
@@ -174,7 +193,7 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	ret = regmap_read(component->regmap, reg, &val);
 	if (ret < 0) {
 		dev_err(dev, "failed to read detected mode: %d\n", ret);
-		return IRQ_NONE;
+		goto error;
 	}
 
 	switch (val) {
@@ -195,7 +214,7 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 		ret = regmap_read(component->regmap, reg, &val);
 		if (ret) {
 			dev_err(dev, "failed to re-check selected mode: %d\n", ret);
-			return IRQ_NONE;
+			goto error;
 		}
 		break;
 	default:
@@ -206,7 +225,7 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 
 	ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL);
 	if (!ucontrol)
-		return IRQ_NONE;
+		goto error;
 
 	ucontrol->value.enumerated.item[0] = snd_soc_enum_val_to_item(soc_enum, val);
 
@@ -215,12 +234,15 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	up_write(rwsem);
 	if (ret < 0) {
 		dev_err(dev, "failed to update selected mode: %d\n", ret);
-		return IRQ_NONE;
+		goto error;
 	}
 
 	snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
 
-	return IRQ_HANDLED;
+	irqret = IRQ_HANDLED;
+error:
+	pm_runtime_put(dev);
+	return irqret;
 }
 
 static int sdca_irq_request_locked(struct device *dev,
-- 
2.47.2

[PATCH 03/15] ASoC: SDCA: Pass SoundWire slave to HID

[Charles Keepax]

The SDCA HID code can't assume that the struct device it is passed is
the SoundWire slave device. HID is represented by a Function in SDCA and
will thus likely be implemented by a child driver. Update the code to
explicitly pass in the SoundWire slave device.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_function.h   |  2 +-
 include/sound/sdca_hid.h        |  8 ++++++--
 sound/soc/sdca/sdca_functions.c | 20 ++++++++++++--------
 sound/soc/sdca/sdca_hid.c       | 12 +++++-------
 4 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index ea68856e4c8c4..51e12fcfc53cd 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -1332,7 +1332,7 @@ static inline u32 sdca_range_search(struct sdca_control_range *range,
 	return 0;
 }
 
-int sdca_parse_function(struct device *dev,
+int sdca_parse_function(struct device *dev, struct sdw_slave *sdw,
 			struct sdca_function_desc *desc,
 			struct sdca_function_data *function);
 
diff --git a/include/sound/sdca_hid.h b/include/sound/sdca_hid.h
index 8ab3e498884ef..3a155835e035e 100644
--- a/include/sound/sdca_hid.h
+++ b/include/sound/sdca_hid.h
@@ -12,10 +12,14 @@
 #include <linux/hid.h>
 
 #if IS_ENABLED(CONFIG_SND_SOC_SDCA_HID)
-int sdca_add_hid_device(struct device *dev, struct sdca_entity *entity);
+
+int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
+			struct sdca_entity *entity);
 
 #else
-static inline int sdca_add_hid_device(struct device *dev, struct sdca_entity *entity)
+
+static inline int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
+				      struct sdca_entity *entity)
 {
 	return 0;
 }
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 13f68f7b6dd6a..7d08e6fc93b83 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -1253,7 +1253,8 @@ static int find_sdca_entity_ge(struct device *dev,
 }
 
 static int
-find_sdca_entity_hide(struct device *dev, struct fwnode_handle *function_node,
+find_sdca_entity_hide(struct device *dev, struct sdw_slave *sdw,
+		      struct fwnode_handle *function_node,
 		      struct fwnode_handle *entity_node, struct sdca_entity *entity)
 {
 	struct sdca_entity_hide *hide = &entity->hide;
@@ -1328,7 +1329,7 @@ find_sdca_entity_hide(struct device *dev, struct fwnode_handle *function_node,
 						      report_desc, nval);
 
 			/* add HID device */
-			ret = sdca_add_hid_device(dev, entity);
+			ret = sdca_add_hid_device(dev, sdw, entity);
 			if (ret) {
 				dev_err(dev, "%pfwP: failed to add HID device: %d\n", entity_node, ret);
 				return ret;
@@ -1339,7 +1340,7 @@ find_sdca_entity_hide(struct device *dev, struct fwnode_handle *function_node,
 	return 0;
 }
 
-static int find_sdca_entity(struct device *dev,
+static int find_sdca_entity(struct device *dev, struct sdw_slave *sdw,
 			    struct fwnode_handle *function_node,
 			    struct fwnode_handle *entity_node,
 			    struct sdca_entity *entity)
@@ -1381,7 +1382,8 @@ static int find_sdca_entity(struct device *dev,
 		ret = find_sdca_entity_ge(dev, entity_node, entity);
 		break;
 	case SDCA_ENTITY_TYPE_HIDE:
-		ret = find_sdca_entity_hide(dev, function_node, entity_node, entity);
+		ret = find_sdca_entity_hide(dev, sdw, function_node,
+					    entity_node, entity);
 		break;
 	default:
 		break;
@@ -1396,7 +1398,7 @@ static int find_sdca_entity(struct device *dev,
 	return 0;
 }
 
-static int find_sdca_entities(struct device *dev,
+static int find_sdca_entities(struct device *dev, struct sdw_slave *sdw,
 			      struct fwnode_handle *function_node,
 			      struct sdca_function_data *function)
 {
@@ -1448,7 +1450,8 @@ static int find_sdca_entities(struct device *dev,
 			return -EINVAL;
 		}
 
-		ret = find_sdca_entity(dev, function_node, entity_node, &entities[i]);
+		ret = find_sdca_entity(dev, sdw, function_node,
+				       entity_node, &entities[i]);
 		fwnode_handle_put(entity_node);
 		if (ret)
 			return ret;
@@ -1927,12 +1930,13 @@ static int find_sdca_clusters(struct device *dev,
 /**
  * sdca_parse_function - parse ACPI DisCo for a Function
  * @dev: Pointer to device against which function data will be allocated.
+ * @sdw: SoundWire slave device to be processed.
  * @function_desc: Pointer to the Function short descriptor.
  * @function: Pointer to the Function information, to be populated.
  *
  * Return: Returns 0 for success.
  */
-int sdca_parse_function(struct device *dev,
+int sdca_parse_function(struct device *dev, struct sdw_slave *sdw,
 			struct sdca_function_desc *function_desc,
 			struct sdca_function_data *function)
 {
@@ -1953,7 +1957,7 @@ int sdca_parse_function(struct device *dev,
 	if (ret)
 		return ret;
 
-	ret = find_sdca_entities(dev, function_desc->node, function);
+	ret = find_sdca_entities(dev, sdw, function_desc->node, function);
 	if (ret)
 		return ret;
 
diff --git a/sound/soc/sdca/sdca_hid.c b/sound/soc/sdca/sdca_hid.c
index 967f7ec6fb79d..53dad1a524d4b 100644
--- a/sound/soc/sdca/sdca_hid.c
+++ b/sound/soc/sdca/sdca_hid.c
@@ -82,15 +82,13 @@ static const struct hid_ll_driver sdw_hid_driver = {
 	.raw_request = sdwhid_raw_request,
 };
 
-int sdca_add_hid_device(struct device *dev, struct sdca_entity *entity)
+int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
+			struct sdca_entity *entity)
 {
-	struct sdw_bus *bus;
+	struct sdw_bus *bus = sdw->bus;
 	struct hid_device *hid;
-	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	int ret;
 
-	bus = slave->bus;
-
 	hid = hid_allocate_device();
 	if (IS_ERR(hid))
 		return PTR_ERR(hid);
@@ -103,8 +101,8 @@ int sdca_add_hid_device(struct device *dev, struct sdca_entity *entity)
 
 	snprintf(hid->name, sizeof(hid->name),
 		 "HID sdw:%01x:%01x:%04x:%04x:%02x",
-		 bus->controller_id, bus->link_id, slave->id.mfg_id,
-		 slave->id.part_id, slave->id.class_id);
+		 bus->controller_id, bus->link_id, sdw->id.mfg_id,
+		 sdw->id.part_id, sdw->id.class_id);
 
 	snprintf(hid->phys, sizeof(hid->phys), "%s", dev->bus->name);
 
-- 
2.47.2

[PATCH 04/15] ASoC: SDCA: Pass device register map from IRQ alloc to handlers

[Charles Keepax]

Store a copy of the device register map in the structure for the IRQ
handlers. This will allow the individual IRQ handlers access to the
device level register map if required.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_interrupts.h  | 2 ++
 sound/soc/sdca/sdca_interrupts.c | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index bbbc3ab27ebae..d652c6e94ddcb 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -23,6 +23,7 @@ struct sdca_function_data;
 /**
  * struct sdca_interrupt - contains information about a single SDCA interrupt
  * @name: The name of the interrupt.
+ * @device_regmap: Pointer to the IRQ regmap.
  * @component: Pointer to the ASoC component owns the interrupt.
  * @function: Pointer to the Function that the interrupt is associated with.
  * @entity: Pointer to the Entity that the interrupt is associated with.
@@ -35,6 +36,7 @@ struct sdca_function_data;
 struct sdca_interrupt {
 	const char *name;
 
+	struct regmap *device_regmap;
 	struct snd_soc_component *component;
 	struct sdca_function_data *function;
 	struct sdca_entity *entity;
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 6775f0f7b5659..944d4713f56d9 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -437,7 +437,7 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
 					      struct regmap *regmap, int irq)
 {
 	struct sdca_interrupt_info *info;
-	int ret;
+	int ret, i;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -445,6 +445,9 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
 
 	info->irq_chip = sdca_irq_chip;
 
+	for (i = 0; i < ARRAY_SIZE(info->irqs); i++)
+		info->irqs[i].device_regmap = regmap;
+
 	ret = devm_mutex_init(dev, &info->irq_lock);
 	if (ret)
 		return ERR_PTR(ret);
-- 
2.47.2

[PATCH 05/15] ASoC: SDCA: Update externally_requested flag to cover all requests

[Charles Keepax]

Currently there is a flag to indicate if an IRQ has been requested by
something outside the SDCA core, such that the core can skip requesting
that IRQ. However, it is simpler and more useful to just note that the
IRQ has been requested. This will allow the core to also request IRQs in
multiple phases.

Rename the externally_requested flag to requested and set it every time
an IRQ is requested.

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

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index d652c6e94ddcb..e2c1337d24e0e 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -29,9 +29,8 @@ struct sdca_function_data;
  * @entity: Pointer to the Entity that the interrupt is associated with.
  * @control: Pointer to the Control that the interrupt is associated with.
  * @priv: Pointer to private data for use by the handler.
- * @externally_requested: Internal flag used to check if a client driver has
- * already requested the interrupt, for custom handling, allowing the core to
- * skip handling this interrupt.
+ * @requested: Internal flag used to check if a client driver has already
+ * requested the interrupt.
  */
 struct sdca_interrupt {
 	const char *name;
@@ -44,7 +43,7 @@ struct sdca_interrupt {
 
 	void *priv;
 
-	bool externally_requested;
+	bool requested;
 };
 
 /**
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 944d4713f56d9..140764931380f 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -262,6 +262,8 @@ static int sdca_irq_request_locked(struct device *dev,
 	if (ret)
 		return ret;
 
+	info->irqs[sdca_irq].requested = true;
+
 	dev_dbg(dev, "requested irq %d for %s\n", irq, name);
 
 	return 0;
@@ -301,8 +303,6 @@ int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
 		return ret;
 	}
 
-	info->irqs[sdca_irq].externally_requested = true;
-
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA");
@@ -379,9 +379,9 @@ int sdca_irq_populate(struct sdca_function_data *function,
 
 			interrupt = &info->irqs[irq];
 
-			if (interrupt->externally_requested) {
+			if (interrupt->requested) {
 				dev_dbg(dev,
-					"skipping irq %d, externally requested\n",
+					"skipping irq %d, already requested\n",
 					irq);
 				continue;
 			}
-- 
2.47.2

[PATCH 06/15] ASoC: SDCA: Factor out a helper to find SDCA IRQ data

[Charles Keepax]

Add a helper function to locate the sdca_interrupt structure for a given
SDCA IRQ, this makes the code a little more readable and will facilitate
some additions in the future.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_interrupts.c | 36 +++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 140764931380f..a203cd2764ecc 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -341,6 +341,24 @@ int sdca_irq_data_populate(struct snd_soc_component *component,
 }
 EXPORT_SYMBOL_NS_GPL(sdca_irq_data_populate, "SND_SOC_SDCA");
 
+static struct sdca_interrupt *get_interrupt_data(struct device *dev, int irq,
+						 struct sdca_interrupt_info *info)
+{
+	if (irq == SDCA_NO_INTERRUPT) {
+		return NULL;
+	} else if (irq < 0 || irq >= SDCA_MAX_INTERRUPTS) {
+		dev_err(dev, "bad irq position: %d\n", irq);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (info->irqs[irq].requested) {
+		dev_dbg(dev, "skipping irq %d, already requested\n", irq);
+		return NULL;
+	}
+
+	return &info->irqs[irq];
+}
+
 /**
  * sdca_irq_populate - Request all the individual IRQs for an SDCA Function
  * @function: Pointer to the SDCA Function.
@@ -370,21 +388,11 @@ int sdca_irq_populate(struct sdca_function_data *function,
 			irq_handler_t handler;
 			int ret;
 
-			if (irq == SDCA_NO_INTERRUPT) {
-				continue;
-			} else if (irq < 0 || irq >= SDCA_MAX_INTERRUPTS) {
-				dev_err(dev, "bad irq position: %d\n", irq);
-				return -EINVAL;
-			}
-
-			interrupt = &info->irqs[irq];
-
-			if (interrupt->requested) {
-				dev_dbg(dev,
-					"skipping irq %d, already requested\n",
-					irq);
+			interrupt = get_interrupt_data(dev, irq, info);
+			if (IS_ERR(interrupt))
+				return PTR_ERR(interrupt);
+			else if (!interrupt)
 				continue;
-			}
 
 			ret = sdca_irq_data_populate(component, function, entity,
 						     control, interrupt);
-- 
2.47.2

[PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Charles Keepax]

In the future some IRQs (mostly File DownLoad) will need to run before
the soundcard is constructed, as such refactor more of the IRQ handling
to use raw device and regmap pointers rather than accessing things
through the component.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_interrupts.h  |  7 +++++-
 sound/soc/sdca/sdca_interrupts.c | 37 +++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index e2c1337d24e0e..4c347fdce1658 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -23,7 +23,9 @@ struct sdca_function_data;
 /**
  * struct sdca_interrupt - contains information about a single SDCA interrupt
  * @name: The name of the interrupt.
+ * @dev: Pointer to the Function device.
  * @device_regmap: Pointer to the IRQ regmap.
+ * @function_regmap: Pointer to the SDCA Function regmap.
  * @component: Pointer to the ASoC component owns the interrupt.
  * @function: Pointer to the Function that the interrupt is associated with.
  * @entity: Pointer to the Entity that the interrupt is associated with.
@@ -35,7 +37,9 @@ struct sdca_function_data;
 struct sdca_interrupt {
 	const char *name;
 
+	struct device *dev;
 	struct regmap *device_regmap;
+	struct regmap *function_regmap;
 	struct snd_soc_component *component;
 	struct sdca_function_data *function;
 	struct sdca_entity *entity;
@@ -65,7 +69,8 @@ struct sdca_interrupt_info {
 int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *interrupt_info,
 		     int sdca_irq, const char *name, irq_handler_t handler,
 		     void *data);
-int sdca_irq_data_populate(struct snd_soc_component *component,
+int sdca_irq_data_populate(struct device *dev, struct regmap *function_regmap,
+			   struct snd_soc_component *component,
 			   struct sdca_function_data *function,
 			   struct sdca_entity *entity,
 			   struct sdca_control *control,
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index a203cd2764ecc..a0677911503fb 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -77,7 +77,7 @@ static const struct regmap_irq_chip sdca_irq_chip = {
 static irqreturn_t base_handler(int irq, void *data)
 {
 	struct sdca_interrupt *interrupt = data;
-	struct device *dev = interrupt->component->dev;
+	struct device *dev = interrupt->dev;
 
 	dev_info(dev, "%s irq without full handling\n", interrupt->name);
 
@@ -87,7 +87,7 @@ static irqreturn_t base_handler(int irq, void *data)
 static irqreturn_t function_status_handler(int irq, void *data)
 {
 	struct sdca_interrupt *interrupt = data;
-	struct device *dev = interrupt->component->dev;
+	struct device *dev = interrupt->dev;
 	irqreturn_t irqret = IRQ_NONE;
 	unsigned int reg, val;
 	unsigned long status;
@@ -103,7 +103,7 @@ static irqreturn_t function_status_handler(int irq, void *data)
 	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
 			   interrupt->control->sel, 0);
 
-	ret = regmap_read(interrupt->component->regmap, reg, &val);
+	ret = regmap_read(interrupt->function_regmap, reg, &val);
 	if (ret < 0) {
 		dev_err(dev, "failed to read function status: %d\n", ret);
 		goto error;
@@ -136,7 +136,7 @@ static irqreturn_t function_status_handler(int irq, void *data)
 		}
 	}
 
-	ret = regmap_write(interrupt->component->regmap, reg, val);
+	ret = regmap_write(interrupt->function_regmap, reg, val);
 	if (ret < 0) {
 		dev_err(dev, "failed to clear function status: %d\n", ret);
 		goto error;
@@ -151,8 +151,8 @@ static irqreturn_t function_status_handler(int irq, void *data)
 static irqreturn_t detected_mode_handler(int irq, void *data)
 {
 	struct sdca_interrupt *interrupt = data;
+	struct device *dev = interrupt->dev;
 	struct snd_soc_component *component = interrupt->component;
-	struct device *dev = component->dev;
 	struct snd_soc_card *card = component->card;
 	struct rw_semaphore *rwsem = &card->snd_card->controls_rwsem;
 	struct snd_kcontrol *kctl = interrupt->priv;
@@ -190,7 +190,7 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
 			   interrupt->control->sel, 0);
 
-	ret = regmap_read(component->regmap, reg, &val);
+	ret = regmap_read(interrupt->function_regmap, reg, &val);
 	if (ret < 0) {
 		dev_err(dev, "failed to read detected mode: %d\n", ret);
 		goto error;
@@ -209,9 +209,9 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 		 * detected mode is unknown we need to see what the device
 		 * selected as a "safe" option.
 		 */
-		regcache_drop_region(component->regmap, reg, reg);
+		regcache_drop_region(interrupt->function_regmap, reg, reg);
 
-		ret = regmap_read(component->regmap, reg, &val);
+		ret = regmap_read(interrupt->function_regmap, reg, &val);
 		if (ret) {
 			dev_err(dev, "failed to re-check selected mode: %d\n", ret);
 			goto error;
@@ -309,6 +309,8 @@ EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA");
 
 /**
  * sdca_irq_data_populate - Populate common interrupt data
+ * @dev: Pointer to the Function device.
+ * @regmap: Pointer to the Function regmap.
  * @component: Pointer to the ASoC component for the Function.
  * @function: Pointer to the SDCA Function.
  * @entity: Pointer to the SDCA Entity.
@@ -317,21 +319,31 @@ EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA");
  *
  * Return: Zero on success, and a negative error code on failure.
  */
-int sdca_irq_data_populate(struct snd_soc_component *component,
+int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
+			   struct snd_soc_component *component,
 			   struct sdca_function_data *function,
 			   struct sdca_entity *entity,
 			   struct sdca_control *control,
 			   struct sdca_interrupt *interrupt)
 {
-	struct device *dev = component->dev;
 	const char *name;
 
+	if (!dev && component)
+		dev = component->dev;
+	if (!dev)
+		return -ENODEV;
+
 	name = devm_kasprintf(dev, GFP_KERNEL, "%s %s %s", function->desc->name,
 			      entity->label, control->label);
 	if (!name)
 		return -ENOMEM;
 
 	interrupt->name = name;
+	interrupt->dev = dev;
+	if (!regmap && component)
+		interrupt->function_regmap = component->regmap;
+	else
+		interrupt->function_regmap = regmap;
 	interrupt->component = component;
 	interrupt->function = function;
 	interrupt->entity = entity;
@@ -394,8 +406,9 @@ int sdca_irq_populate(struct sdca_function_data *function,
 			else if (!interrupt)
 				continue;
 
-			ret = sdca_irq_data_populate(component, function, entity,
-						     control, interrupt);
+			ret = sdca_irq_data_populate(dev, NULL, component,
+						     function, entity, control,
+						     interrupt);
 			if (ret)
 				return ret;
 
-- 
2.47.2

[PATCH 08/15] ASoC: SDCA: Force some SDCA Controls to be volatile

[Charles Keepax]

Whilst SDCA does specify an Access Mode for each Control, there is not a
1-to-1 mapping between that and ASoC's internal representation. Some
registers require being treated as volatile from the hosts perspective
even in their Access Mode is Read-Write. Add an explicit list of SDCA
controls that should be forced volatile.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_function.h   |  1 +
 sound/soc/sdca/sdca_functions.c | 58 +++++++++++++++++++++++++++++++++
 sound/soc/sdca/sdca_regmap.c    |  9 +----
 3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 51e12fcfc53cd..ab9af84082c95 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -771,6 +771,7 @@ struct sdca_control {
 	u8 layers;
 
 	bool deferrable;
+	bool is_volatile;
 	bool has_default;
 	bool has_fixed;
 };
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 7d08e6fc93b83..e12a2a90e34e1 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -779,6 +779,62 @@ find_sdca_control_datatype(const struct sdca_entity *entity,
 	}
 }
 
+static bool find_sdca_control_volatile(const struct sdca_entity *entity,
+				       const struct sdca_control *control)
+{
+	switch (control->mode) {
+	case SDCA_ACCESS_MODE_DC:
+		return false;
+	case SDCA_ACCESS_MODE_RO:
+	case SDCA_ACCESS_MODE_RW1S:
+	case SDCA_ACCESS_MODE_RW1C:
+		return true;
+	default:
+		break;
+	}
+
+	switch (SDCA_CTL_TYPE(entity->type, control->sel)) {
+	case SDCA_CTL_TYPE_S(XU, FDL_HOST_REQUEST):
+	case SDCA_CTL_TYPE_S(XU, FDL_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(XU, FDL_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SPE, AUTHTX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SPE, AUTHTX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SPE, AUTHRX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SPE, AUTHRX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(MFPU, AE_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(MFPU, AE_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SMPU, HIST_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SMPU, HIST_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SMPU, DTODTX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SMPU, DTODTX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SMPU, DTODRX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SMPU, DTODRX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SAPU, DTODTX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SAPU, DTODTX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(SAPU, DTODRX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(SAPU, DTODRX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(HIDE, HIDTX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(HIDE, HIDTX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(HIDE, HIDRX_MESSAGEOFFSET):
+	case SDCA_CTL_TYPE_S(HIDE, HIDRX_MESSAGELENGTH):
+	case SDCA_CTL_TYPE_S(XU, FDL_STATUS):
+	case SDCA_CTL_TYPE_S(XU, FDL_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SPE, AUTHTX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SPE, AUTHRX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(MFPU, AE_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SMPU, HIST_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SMPU, DTODTX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SMPU, DTODRX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SAPU, DTODTX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(SAPU, DTODRX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(HIDE, HIDTX_CURRENTOWNER):
+	case SDCA_CTL_TYPE_S(HIDE, HIDRX_CURRENTOWNER):
+		return true;
+	default:
+		return false;
+	}
+}
+
 static int find_sdca_control_range(struct device *dev,
 				   struct fwnode_handle *control_node,
 				   struct sdca_control_range *range)
@@ -930,6 +986,8 @@ static int find_sdca_entity_control(struct device *dev, struct sdca_entity *enti
 		break;
 	}
 
+	control->is_volatile = find_sdca_control_volatile(entity, control);
+
 	ret = find_sdca_control_range(dev, control_node, &control->range);
 	if (ret) {
 		dev_err(dev, "%s: control %#x: range missing: %d\n",
diff --git a/sound/soc/sdca/sdca_regmap.c b/sound/soc/sdca/sdca_regmap.c
index 72f893e00ff50..8fa138fca00ff 100644
--- a/sound/soc/sdca/sdca_regmap.c
+++ b/sound/soc/sdca/sdca_regmap.c
@@ -147,14 +147,7 @@ bool sdca_regmap_volatile(struct sdca_function_data *function, unsigned int reg)
 	if (!control)
 		return false;
 
-	switch (control->mode) {
-	case SDCA_ACCESS_MODE_RO:
-	case SDCA_ACCESS_MODE_RW1S:
-	case SDCA_ACCESS_MODE_RW1C:
-		return true;
-	default:
-		return false;
-	}
+	return control->is_volatile;
 }
 EXPORT_SYMBOL_NS(sdca_regmap_volatile, "SND_SOC_SDCA");
 
-- 
2.47.2

[PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

Add helper functions for handling Universal Message Passing (UMP)
buffers on SDCA devices. These are generic mechanisms to pass blocks of
binary data between the host and the device, in both directions. They
are used for things like passing HID descriptors and the File Download
process.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_function.h |  26 ++++
 include/sound/sdca_ump.h      |  45 +++++++
 sound/soc/sdca/Makefile       |   3 +-
 sound/soc/sdca/sdca_ump.c     | 247 ++++++++++++++++++++++++++++++++++
 4 files changed, 320 insertions(+), 1 deletion(-)
 create mode 100644 include/sound/sdca_ump.h
 create mode 100644 sound/soc/sdca/sdca_ump.c

diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index ab9af84082c95..341912d896679 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -133,6 +133,32 @@ struct sdca_init_write {
 #define SDCA_CTL_TYPE_S(ent, sel) SDCA_CTL_TYPE(SDCA_ENTITY_TYPE_##ent, \
 						SDCA_CTL_##ent##_##sel)
 
+/**
+ * enum sdca_messageoffset_range - Column definitions UMP MessageOffset
+ */
+enum sdca_messageoffset_range {
+	SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS		= 0,
+	SDCA_MESSAGEOFFSET_BUFFER_LENGTH		= 1,
+	SDCA_MESSAGEOFFSET_UMP_MODE			= 2,
+	SDCA_MESSAGEOFFSET_NCOLS			= 3,
+};
+
+/**
+ * enum sdca_ump_mode - SDCA UMP Mode
+ */
+enum sdca_ump_mode {
+	SDCA_UMP_MODE_DIRECT				= 0x00,
+	SDCA_UMP_MODE_INDIRECT				= 0x01,
+};
+
+/**
+ * enum sdca_ump_owner - SDCA UMP Owner
+ */
+enum sdca_ump_owner {
+	SDCA_UMP_OWNER_HOST				= 0x00,
+	SDCA_UMP_OWNER_DEVICE				= 0x01,
+};
+
 /**
  * enum sdca_it_controls - SDCA Controls for Input Terminal
  *
diff --git a/include/sound/sdca_ump.h b/include/sound/sdca_ump.h
new file mode 100644
index 0000000000000..b2363199d19aa
--- /dev/null
+++ b/include/sound/sdca_ump.h
@@ -0,0 +1,45 @@
+/* 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_UMP_H__
+#define __SDCA_UMP_H__
+
+struct regmap;
+struct sdca_control;
+struct sdca_entity;
+struct sdca_function_data;
+struct snd_soc_component;
+
+int sdca_ump_get_owner_host(struct device *dev,
+			    struct regmap *function_regmap,
+			    struct sdca_function_data *function,
+			    struct sdca_entity *entity,
+			    struct sdca_control *control);
+int sdca_ump_set_owner_device(struct device *dev,
+			      struct regmap *function_regmap,
+			      struct sdca_function_data *function,
+			      struct sdca_entity *entity,
+			      struct sdca_control *control);
+int sdca_ump_read_message(struct device *dev,
+			  struct regmap *device_regmap,
+			  struct regmap *function_regmap,
+			  struct sdca_function_data *function,
+			  struct sdca_entity *entity,
+			  unsigned int offset_sel, unsigned int length_sel,
+			  void **msg);
+int sdca_ump_write_message(struct device *dev,
+			   struct regmap *device_regmap,
+			   struct regmap *function_regmap,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   unsigned int offset_sel, unsigned int msg_offset,
+			   unsigned int length_sel,
+			   void *msg, int msg_len);
+
+#endif // __SDCA_UMP_H__
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index 5e51760cb6513..a1b24c95cd8c8 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -1,6 +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
+snd-soc-sdca-y := sdca_functions.o sdca_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
 
diff --git a/sound/soc/sdca/sdca_ump.c b/sound/soc/sdca/sdca_ump.c
new file mode 100644
index 0000000000000..5dcad2f7ea05b
--- /dev/null
+++ b/sound/soc/sdca/sdca_ump.c
@@ -0,0 +1,247 @@
+// 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/dev_printk.h>
+#include <linux/device.h>
+#include <linux/regmap.h>
+#include <sound/sdca.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_ump.h>
+#include <sound/soc-component.h>
+#include <linux/soundwire/sdw_registers.h>
+
+/**
+ * sdca_ump_get_owner_host - check a UMP buffer is owned by the host
+ * @dev: Pointer to the struct device used for error messages.
+ * @function_regmap: Pointer to the regmap for the SDCA Function.
+ * @function: Pointer to the Function information.
+ * @entity: Pointer to the SDCA Entity.
+ * @control: Pointer to the SDCA Control for the UMP Owner.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_ump_get_owner_host(struct device *dev,
+			    struct regmap *function_regmap,
+			    struct sdca_function_data *function,
+			    struct sdca_entity *entity,
+			    struct sdca_control *control)
+{
+	unsigned int reg, owner;
+	int ret;
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
+	ret = regmap_read(function_regmap, reg, &owner);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to read UMP owner: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	if (owner != SDCA_UMP_OWNER_HOST) {
+		dev_err(dev, "%s: host is not the UMP owner\n", entity->label);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_ump_get_owner_host, "SND_SOC_SDCA");
+
+/**
+ * sdca_ump_set_owner_device - set a UMP buffer's ownership back to the device
+ * @dev: Pointer to the struct device used for error messages.
+ * @function_regmap: Pointer to the regmap for the SDCA Function.
+ * @function: Pointer to the Function information.
+ * @entity: Pointer to the SDCA Entity.
+ * @control: Pointer to the SDCA Control for the UMP Owner.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_ump_set_owner_device(struct device *dev,
+			      struct regmap *function_regmap,
+			      struct sdca_function_data *function,
+			      struct sdca_entity *entity,
+			      struct sdca_control *control)
+{
+	unsigned int reg;
+	int ret;
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
+	ret = regmap_write(function_regmap, reg, SDCA_UMP_OWNER_DEVICE);
+	if (ret < 0)
+		dev_err(dev, "%s: failed to write UMP owner: %d\n",
+			entity->label, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_ump_set_owner_device, "SND_SOC_SDCA");
+
+/**
+ * sdca_ump_read_message - read a UMP message from the device
+ * @dev: Pointer to the struct device used for error messages.
+ * @device_regmap: Pointer to the Device register map.
+ * @function_regmap: Pointer to the regmap for the SDCA Function.
+ * @function: Pointer to the Function information.
+ * @entity: Pointer to the SDCA Entity.
+ * @offset_sel: Control Selector for the UMP Offset Control.
+ * @length_sel: Control Selector for the UMP Length Control.
+ * @msg: Pointer that will be populated with an dynamically buffer
+ * containing the UMP message. Note this needs to be freed by the
+ * caller.
+ *
+ * The caller should first call sdca_ump_get_owner_host() to ensure the host
+ * currently owns the UMP buffer, and then this function can be used to
+ * retrieve a message. It is the callers responsibility to free the
+ * message once it is finished with it. Finally sdca_ump_set_owner_device()
+ * should be called to return the buffer to the device.
+ *
+ * Return: Returns the message length on success, and a negative error
+ * code on failure.
+ */
+int sdca_ump_read_message(struct device *dev,
+			  struct regmap *device_regmap,
+			  struct regmap *function_regmap,
+			  struct sdca_function_data *function,
+			  struct sdca_entity *entity,
+			  unsigned int offset_sel, unsigned int length_sel,
+			  void **msg)
+{
+	struct sdca_control_range *range;
+	unsigned int msg_offset, msg_len;
+	unsigned int buf_addr, buf_len;
+	unsigned int reg;
+	int ret;
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, offset_sel, 0);
+	ret = regmap_read(function_regmap, reg, &msg_offset);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to read UMP offset: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	range = sdca_selector_find_range(dev, entity, offset_sel,
+					 SDCA_MESSAGEOFFSET_NCOLS, 1);
+	if (!range)
+		return -ENOENT;
+
+	buf_addr = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS, 0);
+	buf_len = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_LENGTH, 0);
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, length_sel, 0);
+	ret = regmap_read(function_regmap, reg, &msg_len);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to read UMP length: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	if (msg_len > buf_len - msg_offset) {
+		dev_err(dev, "%s: message too big for UMP buffer: %d\n",
+			entity->label, msg_len);
+		return -EINVAL;
+	}
+
+	*msg = kmalloc(msg_len, GFP_KERNEL);
+	if (!*msg)
+		return -ENOMEM;
+
+	ret = regmap_raw_read(device_regmap, buf_addr + msg_offset, *msg, msg_len);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to read UMP message: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	return msg_len;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_ump_read_message, "SND_SOC_SDCA");
+
+/**
+ * sdca_ump_write_message - write a UMP message to the device
+ * @dev: Pointer to the struct device used for error messages.
+ * @device_regmap: Pointer to the Device register map.
+ * @function_regmap: Pointer to the regmap for the SDCA Function.
+ * @function: Pointer to the Function information.
+ * @entity: Pointer to the SDCA Entity.
+ * @offset_sel: Control Selector for the UMP Offset Control.
+ * @msg_offset: Offset within the UMP buffer at which the message should
+ * be written.
+ * @length_sel: Control Selector for the UMP Length Control.
+ * @msg: Pointer to the data that should be written to the UMP buffer.
+ * @msg_len: Length of the message data in bytes.
+ *
+ * The caller should first call sdca_ump_get_owner_host() to ensure the host
+ * currently owns the UMP buffer, and then this function can be used to
+ * write a message. Finally sdca_ump_set_owner_device() should be called to
+ * return the buffer to the device, allowing the device to access the
+ * message.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_ump_write_message(struct device *dev,
+			   struct regmap *device_regmap,
+			   struct regmap *function_regmap,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   unsigned int offset_sel, unsigned int msg_offset,
+			   unsigned int length_sel,
+			   void *msg, int msg_len)
+{
+	struct sdca_control_range *range;
+	unsigned int buf_addr, buf_len, ump_mode;
+	unsigned int reg;
+	int ret;
+
+	range = sdca_selector_find_range(dev, entity, offset_sel,
+					 SDCA_MESSAGEOFFSET_NCOLS, 1);
+	if (!range)
+		return -ENOENT;
+
+	buf_addr = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS, 0);
+	buf_len = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_LENGTH, 0);
+	ump_mode = sdca_range(range, SDCA_MESSAGEOFFSET_UMP_MODE, 0);
+
+	if (msg_len > buf_len - msg_offset) {
+		dev_err(dev, "%s: message too big for UMP buffer: %d\n",
+			entity->label, msg_len);
+		return -EINVAL;
+	}
+
+	if (ump_mode != SDCA_UMP_MODE_DIRECT) {
+		dev_err(dev, "%s: only direct mode currently supported\n",
+			entity->label);
+		return -EINVAL;
+	}
+
+	ret = regmap_raw_write(device_regmap, buf_addr + msg_offset, msg, msg_len);
+	if (ret) {
+		dev_err(dev, "%s: failed to write UMP message: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, offset_sel, 0);
+	ret = regmap_write(function_regmap, reg, msg_offset);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to write UMP offset: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, length_sel, 0);
+	ret = regmap_write(function_regmap, reg, msg_len);
+	if (ret < 0) {
+		dev_err(dev, "%s: failed to write UMP length: %d\n",
+			entity->label, ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_ump_write_message, "SND_SOC_SDCA");
-- 
2.47.2

[PATCH 10/15] ASoC: SDCA: Add SDCA FDL data parsing

[Charles Keepax]

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

Add parsing of ACPI DisCo information specific to FDL (File DownLoad).
DisCo contains a list of File Sets which can be requested by the device
and within each of those a list of individual files to be downloaded to
the device. Optionally the contents of the files may also be present in
a special ACPI table, called SWFT (SoundWire File Table).

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
---
 include/sound/sdca.h            |  5 ++
 include/sound/sdca_function.h   | 40 ++++++++++++++
 sound/soc/sdca/sdca_device.c    | 20 +++++++
 sound/soc/sdca/sdca_functions.c | 93 +++++++++++++++++++++++++++++++++
 4 files changed, 158 insertions(+)

diff --git a/include/sound/sdca.h b/include/sound/sdca.h
index 9c6a351c9d474..d38cdbfeb35f5 100644
--- a/include/sound/sdca.h
+++ b/include/sound/sdca.h
@@ -12,6 +12,7 @@
 #include <linux/types.h>
 #include <linux/kconfig.h>
 
+struct acpi_table_swft;
 struct sdw_slave;
 
 #define SDCA_MAX_FUNCTION_COUNT 8
@@ -37,11 +38,13 @@ struct sdca_function_desc {
  * @num_functions: Total number of supported SDCA functions. Invalid/unsupported
  * functions will be skipped.
  * @function: Array of function descriptors.
+ * @swft: Pointer to the SWFT table, if available.
  */
 struct sdca_device_data {
 	u32 interface_revision;
 	int num_functions;
 	struct sdca_function_desc function[SDCA_MAX_FUNCTION_COUNT];
+	struct acpi_table_swft *swft;
 };
 
 enum sdca_quirk {
@@ -52,12 +55,14 @@ enum sdca_quirk {
 #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_SOC_SDCA)
 
 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);
 
 #else
 
 static inline void sdca_lookup_functions(struct sdw_slave *slave) {}
+static inline void sdca_lookup_swft(struct sdw_slave *slave) {}
 static inline void sdca_lookup_interface_revision(struct sdw_slave *slave) {}
 static inline bool sdca_device_quirk_match(struct sdw_slave *slave, enum sdca_quirk quirk)
 {
diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 341912d896679..7ee2f6666958b 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -13,6 +13,7 @@
 #include <linux/types.h>
 #include <linux/hid.h>
 
+struct acpi_table_swft;
 struct device;
 struct sdca_entity;
 struct sdca_function_desc;
@@ -1315,6 +1316,42 @@ enum sdca_cluster_range {
 	SDCA_CLUSTER_NCOLS				= 2,
 };
 
+/**
+ * struct sdca_fdl_file - information about a file from a fileset used in FDL
+ * @vendor_id: Vendor ID of the file.
+ * @file_id: File ID of the file.
+ * @fdl_offset: Offset information for FDL.
+ */
+struct sdca_fdl_file {
+	u16 vendor_id;
+	u32 file_id;
+	u32 fdl_offset;
+};
+
+/**
+ * struct sdca_fdl_set - information about a set of files used in FDL
+ * @files: Array of files in this FDL set.
+ * @num_files: Number of files in this FDL set.
+ * @id: ID of the FDL set.
+ */
+struct sdca_fdl_set {
+	struct sdca_fdl_file *files;
+	int num_files;
+	u32 id;
+};
+
+/**
+ * struct sdca_fdl_data - information about a function's FDL data
+ * @swft: Pointer to the SoundWire File Table.
+ * @sets: Array of FDL sets used by this function.
+ * @num_sets: Number of FDL sets used by this function.
+ */
+struct sdca_fdl_data {
+	struct acpi_table_swft *swft;
+	struct sdca_fdl_set *sets;
+	int num_sets;
+};
+
 /**
  * struct sdca_function_data - top-level information for one SDCA function
  * @desc: Pointer to short descriptor from initial parsing.
@@ -1326,6 +1363,7 @@ enum sdca_cluster_range {
  * @num_clusters: Number of Channel Clusters reported in this Function.
  * @busy_max_delay: Maximum Function busy delay in microseconds, before an
  * error should be reported.
+ * @fdl_data: FDL data for this Function, if available.
  */
 struct sdca_function_data {
 	struct sdca_function_desc *desc;
@@ -1338,6 +1376,8 @@ struct sdca_function_data {
 	int num_clusters;
 
 	unsigned int busy_max_delay;
+
+	struct sdca_fdl_data fdl_data;
 };
 
 static inline u32 sdca_range(struct sdca_control_range *range,
diff --git a/sound/soc/sdca/sdca_device.c b/sound/soc/sdca/sdca_device.c
index 4798ce2c8f0b4..405e80b979de8 100644
--- a/sound/soc/sdca/sdca_device.c
+++ b/sound/soc/sdca/sdca_device.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/property.h>
@@ -27,6 +28,25 @@ void sdca_lookup_interface_revision(struct sdw_slave *slave)
 }
 EXPORT_SYMBOL_NS(sdca_lookup_interface_revision, "SND_SOC_SDCA");
 
+static void devm_acpi_table_put(void *ptr)
+{
+	acpi_put_table((struct acpi_table_header *)ptr);
+}
+
+void sdca_lookup_swft(struct sdw_slave *slave)
+{
+	acpi_status status;
+
+	status = acpi_get_table(ACPI_SIG_SWFT, 0,
+				(struct acpi_table_header **)&slave->sdca_data.swft);
+	if (ACPI_FAILURE(status))
+		dev_info(&slave->dev, "SWFT not available\n");
+	else
+		devm_add_action_or_reset(&slave->dev, devm_acpi_table_put,
+					 &slave->sdca_data.swft);
+}
+EXPORT_SYMBOL_NS(sdca_lookup_swft, "SND_SOC_SDCA");
+
 static bool sdca_device_quirk_rt712_vb(struct sdw_slave *slave)
 {
 	struct sdw_slave_id *id = &slave->id;
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index e12a2a90e34e1..e1e1ab23e0841 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -1398,6 +1398,95 @@ find_sdca_entity_hide(struct device *dev, struct sdw_slave *sdw,
 	return 0;
 }
 
+static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
+			      struct fwnode_handle *function_node,
+			      struct sdca_function_data *function)
+{
+	static const int mult_fileset = 3;
+	char fileset_name[SDCA_PROPERTY_LENGTH];
+	u32 *filesets_list __free(kfree) = NULL;
+	struct sdca_fdl_set *sets;
+	int num_sets;
+	int i, j;
+
+	num_sets = fwnode_property_count_u32(function_node,
+					     "mipi-sdca-file-set-id-list");
+	if (num_sets == 0 || num_sets == -EINVAL) {
+		return 0;
+	} else if (num_sets < 0) {
+		dev_err(dev, "%pfwP: failed to read file set list: %d\n",
+			function_node, num_sets);
+		return num_sets;
+	}
+
+	filesets_list = kcalloc(num_sets, sizeof(u32), GFP_KERNEL);
+	if (!filesets_list)
+		return -ENOMEM;
+
+	fwnode_property_read_u32_array(function_node, "mipi-sdca-file-set-id-list",
+				       filesets_list, num_sets);
+
+	sets = devm_kcalloc(dev, num_sets, sizeof(struct sdca_fdl_set), GFP_KERNEL);
+	if (!sets)
+		return -ENOMEM;
+
+	for (i = 0; i < num_sets; i++) {
+		u32 *fileset_entries __free(kfree) = NULL;
+		struct sdca_fdl_set *set = &sets[i];
+		struct sdca_fdl_file *files;
+		int num_files, num_entries;
+
+		snprintf(fileset_name, sizeof(fileset_name),
+			 "mipi-sdca-file-set-id-0x%X", filesets_list[i]);
+
+		num_entries = fwnode_property_count_u32(function_node, fileset_name);
+		if (num_entries <= 0) {
+			dev_err(dev, "%pfwP: file set %d missing entries: %d\n",
+				function_node, filesets_list[i], num_entries);
+			return -EINVAL;
+		} else if (num_entries % mult_fileset != 0) {
+			dev_err(dev, "%pfwP: file set %d files not multiple of %d\n",
+				function_node, filesets_list[i], mult_fileset);
+			return -EINVAL;
+		}
+
+		dev_info(dev, "fileset: %#x\n", filesets_list[i]);
+
+		files = devm_kcalloc(dev, num_entries / mult_fileset,
+				     sizeof(struct sdca_fdl_file), GFP_KERNEL);
+		if (!files)
+			return -ENOMEM;
+
+		fileset_entries = kcalloc(num_entries, sizeof(u32), GFP_KERNEL);
+		if (!fileset_entries)
+			return -ENOMEM;
+
+		fwnode_property_read_u32_array(function_node, fileset_name,
+					       fileset_entries, num_entries);
+
+		for (j = 0, num_files = 0; j < num_entries; num_files++) {
+			struct sdca_fdl_file *file = &files[num_files];
+
+			file->vendor_id = fileset_entries[j++];
+			file->file_id = fileset_entries[j++];
+			file->fdl_offset = fileset_entries[j++];
+
+			dev_info(dev, "file: %#x, vendor: %#x, offset: %#x\n",
+				 file->file_id, file->vendor_id, file->fdl_offset);
+		}
+
+		set->id = filesets_list[i];
+		set->num_files = num_files;
+		set->files = files;
+	}
+
+	function->fdl_data.swft = sdw->sdca_data.swft;
+	function->fdl_data.num_sets = num_sets;
+	function->fdl_data.sets = sets;
+
+	return 0;
+}
+
 static int find_sdca_entity(struct device *dev, struct sdw_slave *sdw,
 			    struct fwnode_handle *function_node,
 			    struct fwnode_handle *entity_node,
@@ -2027,6 +2116,10 @@ int sdca_parse_function(struct device *dev, struct sdw_slave *sdw,
 	if (ret < 0)
 		return ret;
 
+	ret = find_sdca_filesets(dev, sdw, function_desc->node, function);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 EXPORT_SYMBOL_NS(sdca_parse_function, "SND_SOC_SDCA");
-- 
2.47.2

[PATCH 11/15] ASoC: SDCA: Add FDL library for XU entities

[Charles Keepax]

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

Some instances of the XU Entity have a need for Files to be downloaded
from the Host. In these XUs, there is one instance of a Host to Device
(Consumer) UMP, identified by the FDL_CurrentOwner Control. XU Library
introduced here implements the FDL flow triggered by FDL_CurrentOwner
irq, which sends a file from SoundWire File Table (SWFT) or from the
firmware directory in specific cases, to the Device FDL UMP.

Currently only Direct method of FDL is implemented.

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
---
 include/sound/sdca_entity_xu.h  |  52 ++++++
 include/sound/sdca_function.h   |  21 +++
 sound/soc/sdca/Kconfig          |   7 +
 sound/soc/sdca/Makefile         |   1 +
 sound/soc/sdca/sdca_entity_xu.c | 315 ++++++++++++++++++++++++++++++++
 5 files changed, 396 insertions(+)
 create mode 100644 include/sound/sdca_entity_xu.h
 create mode 100644 sound/soc/sdca/sdca_entity_xu.c

diff --git a/include/sound/sdca_entity_xu.h b/include/sound/sdca_entity_xu.h
new file mode 100644
index 0000000000000..6f2ab0fc905a8
--- /dev/null
+++ b/include/sound/sdca_entity_xu.h
@@ -0,0 +1,52 @@
+/* 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_ENTITY_XU_H__
+#define __SDCA_ENTITY_XU_H__
+
+struct sdca_fdl_set;
+struct sdca_interrupt;
+
+/**
+ * struct fdl_state - FDL state structure to keep data between interrupts
+ * @set: Pointer to the FDL set currently being downloaded.
+ * @file_index: Index of the current file being processed.
+ */
+struct fdl_state {
+	struct sdca_fdl_set *set;
+	int file_index;
+};
+
+#define SDCA_CTL_XU_FDLH_COMPLETE	0
+#define SDCA_CTL_XU_FDLH_MORE_FILES	SDCA_CTL_XU_FDLH_SET_IN_PROGRESS
+#define SDCA_CTL_XU_FDLH_FILE_AVAILABLE	(SDCA_CTL_XU_FDLH_TRANSFERRED_FILE | \
+					 SDCA_CTL_XU_FDLH_SET_IN_PROGRESS)
+#define SDCA_CTL_XU_FDLH_MASK		(SDCA_CTL_XU_FDLH_TRANSFERRED_CHUNK | \
+					 SDCA_CTL_XU_FDLH_TRANSFERRED_FILE | \
+					 SDCA_CTL_XU_FDLH_SET_IN_PROGRESS | \
+					 SDCA_CTL_XU_FDLH_RESET_ACK | \
+					 SDCA_CTL_XU_FDLH_REQ_ABORT)
+
+#define SDCA_CTL_XU_FDLD_COMPLETE	0
+#define SDCA_CTL_XU_FDLD_FILE_OK	(SDCA_CTL_XU_FDLH_TRANSFERRED_FILE | \
+					 SDCA_CTL_XU_FDLH_SET_IN_PROGRESS | \
+					 SDCA_CTL_XU_FDLD_ACK_TRANSFER | \
+					 SDCA_CTL_XU_FDLD_NEEDS_SET)
+#define SDCA_CTL_XU_FDLD_MORE_FILES_OK	(SDCA_CTL_XU_FDLH_SET_IN_PROGRESS | \
+					 SDCA_CTL_XU_FDLD_ACK_TRANSFER | \
+					 SDCA_CTL_XU_FDLD_NEEDS_SET)
+#define SDCA_CTL_XU_FDLD_MASK		(SDCA_CTL_XU_FDLD_REQ_RESET | \
+					 SDCA_CTL_XU_FDLD_REQ_ABORT | \
+					 SDCA_CTL_XU_FDLD_ACK_TRANSFER | \
+					 SDCA_CTL_XU_FDLD_NEEDS_SET)
+
+int sdca_fdl_alloc_state(struct sdca_interrupt *interrupt);
+int sdca_fdl_process(struct sdca_interrupt *interrupt);
+
+#endif // __SDCA_ENTITY_XU_H__
diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 7ee2f6666958b..7578615b8dbdf 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -285,6 +285,27 @@ enum sdca_xu_controls {
 	SDCA_CTL_XU_FDL_STATUS				= 0x14,
 	SDCA_CTL_XU_FDL_SET_INDEX			= 0x15,
 	SDCA_CTL_XU_FDL_HOST_REQUEST			= 0x16,
+
+	/* FDL Status Host->Device bit definitions */
+	SDCA_CTL_XU_FDLH_TRANSFERRED_CHUNK		= BIT(0),
+	SDCA_CTL_XU_FDLH_TRANSFERRED_FILE		= BIT(1),
+	SDCA_CTL_XU_FDLH_SET_IN_PROGRESS		= BIT(2),
+	SDCA_CTL_XU_FDLH_RESET_ACK			= BIT(4),
+	SDCA_CTL_XU_FDLH_REQ_ABORT			= BIT(5),
+	/* FDL Status Device->Host bit definitions */
+	SDCA_CTL_XU_FDLD_REQ_RESET			= BIT(4),
+	SDCA_CTL_XU_FDLD_REQ_ABORT			= BIT(5),
+	SDCA_CTL_XU_FDLD_ACK_TRANSFER			= BIT(6),
+	SDCA_CTL_XU_FDLD_NEEDS_SET			= BIT(7),
+};
+
+/**
+ * enum sdca_set_index_range - Column definitions UMP SetIndex
+ */
+enum sdca_fdl_set_index_range {
+	SDCA_FDL_SET_INDEX_SET_NUMBER			= 0,
+	SDCA_FDL_SET_INDEX_FILE_SET_ID			= 1,
+	SDCA_FDL_SET_INDEX_NCOLS			= 2,
 };
 
 /**
diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
index 6a3ba43f26bd9..a607e2a28fc67 100644
--- a/sound/soc/sdca/Kconfig
+++ b/sound/soc/sdca/Kconfig
@@ -25,6 +25,13 @@ config SND_SOC_SDCA_IRQ
 	help
 	  This option enables support for SDCA IRQs.
 
+config SND_SOC_SDCA_XU
+	bool "SDCA XU support"
+	depends on SND_SOC_SDCA
+	default y
+	help
+	  This option enables support for the MIPI SDCA XU Entity driver.
+
 config SND_SOC_SDCA_OPTIONAL
 	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
 
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index a1b24c95cd8c8..f2155eb6b6f98 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -4,5 +4,6 @@ snd-soc-sdca-y := sdca_functions.o sdca_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_XU) += sdca_entity_xu.o
 
 obj-$(CONFIG_SND_SOC_SDCA) += snd-soc-sdca.o
diff --git a/sound/soc/sdca/sdca_entity_xu.c b/sound/soc/sdca/sdca_entity_xu.c
new file mode 100644
index 0000000000000..ff7758460edc3
--- /dev/null
+++ b/sound/soc/sdca/sdca_entity_xu.c
@@ -0,0 +1,315 @@
+// 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/acpi.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/dmi.h>
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/regmap.h>
+#include <linux/sprintf.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <sound/sdca.h>
+#include <sound/sdca_entity_xu.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/sdca_ump.h>
+
+/**
+ * sdca_fdl_alloc_state - allocate state for an FDL interrupt
+ * @interrupt: SDCA interrupt structure.
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_fdl_alloc_state(struct sdca_interrupt *interrupt)
+{
+	struct device *dev = interrupt->dev;
+	struct fdl_state *fdl_state;
+
+	fdl_state = devm_kzalloc(dev, sizeof(struct fdl_state), GFP_KERNEL);
+	if (!fdl_state)
+		return -ENOMEM;
+
+	interrupt->priv = fdl_state;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_fdl_alloc_state, "SND_SOC_SDCA");
+
+static char *fdl_get_sku_filename(struct device *dev,
+				  struct sdca_fdl_file *fdl_file)
+{
+	struct device *parent = dev;
+	const char *product_vendor;
+	const char *product_sku;
+
+	/*
+	 * Try to find pci_dev manually because the card may not be ready to be
+	 * used for snd_soc_card_get_pci_ssid yet
+	 */
+	while (parent) {
+		if (dev_is_pci(parent)) {
+			struct pci_dev *pci_dev = to_pci_dev(parent);
+
+			return kasprintf(GFP_KERNEL, "sdca/%x/%x/%x/%x.bin",
+					 fdl_file->vendor_id,
+					 pci_dev->subsystem_vendor,
+					 pci_dev->subsystem_device,
+					 fdl_file->file_id);
+		} else {
+			parent = parent->parent;
+		}
+	}
+
+	product_vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (!product_vendor || !strcmp(product_vendor, "Default string"))
+		product_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
+	if (!product_vendor || !strcmp(product_vendor, "Default string"))
+		product_vendor = dmi_get_system_info(DMI_CHASSIS_VENDOR);
+	if (!product_vendor)
+		product_vendor = "unknown";
+
+	product_sku = dmi_get_system_info(DMI_PRODUCT_SKU);
+	if (!product_sku || !strcmp(product_sku, "Default string"))
+		product_sku = dmi_get_system_info(DMI_PRODUCT_NAME);
+	if (!product_sku)
+		product_sku = "unknown";
+
+	return kasprintf(GFP_KERNEL, "sdca/%x/%s/%s/%x.bin", fdl_file->vendor_id,
+			 product_vendor, product_sku, fdl_file->file_id);
+}
+
+static int fdl_load_file(struct sdca_interrupt *interrupt,
+			 struct sdca_fdl_set *set, int file_index)
+{
+	struct device *dev = interrupt->dev;
+	struct sdca_fdl_data *fdl_data = &interrupt->function->fdl_data;
+	const struct firmware *firmware = NULL;
+	struct acpi_sw_file *swf = NULL, *tmp;
+	struct sdca_fdl_file *fdl_file;
+	char *disk_filename;
+	int ret;
+	int i;
+
+	if (!set) {
+		dev_err(dev, "request to load file with no set\n");
+		return -EINVAL;
+	}
+
+	fdl_file = &set->files[file_index];
+
+	if (fdl_data->swft) {
+		tmp = fdl_data->swft->files;
+		for (i = 0; i < fdl_data->swft->header.length; i += tmp->file_length,
+		     tmp = ACPI_ADD_PTR(struct acpi_sw_file, tmp, tmp->file_length)) {
+			if (tmp->vendor_id == fdl_file->vendor_id &&
+			    tmp->file_id == fdl_file->file_id) {
+				dev_dbg(dev, "located SWF in ACPI: %x-%x-%x\n",
+					tmp->vendor_id, tmp->file_id,
+					tmp->file_version);
+				swf = tmp;
+				break;
+			}
+		}
+	}
+
+	disk_filename = fdl_get_sku_filename(dev, fdl_file);
+	if (!disk_filename)
+		return -ENOMEM;
+
+	dev_dbg(dev, "FDL disk filename: %s\n", disk_filename);
+
+	ret = firmware_request_nowarn(&firmware, disk_filename, dev);
+	kfree(disk_filename);
+	if (ret) {
+		disk_filename = kasprintf(GFP_KERNEL, "sdca/%x/%x.bin",
+					  fdl_file->vendor_id, fdl_file->file_id);
+		if (!disk_filename)
+			return -ENOMEM;
+
+		dev_dbg(dev, "FDL disk filename: %s\n", disk_filename);
+
+		ret = firmware_request_nowarn(&firmware, disk_filename, dev);
+		kfree(disk_filename);
+	}
+
+	if (!ret) {
+		tmp = (struct acpi_sw_file *)&firmware->data[0];
+
+		if (!swf || swf->file_version <= tmp->file_version) {
+			dev_dbg(dev, "using SWF from disk: %x-%x-%x\n",
+				tmp->vendor_id, tmp->file_id, tmp->file_version);
+			swf = tmp;
+		}
+	} else if (!swf) {
+		dev_err(dev, "failed to locate SWF\n");
+		return -ENOENT;
+	}
+
+	ret = sdca_ump_write_message(dev, interrupt->device_regmap,
+				     interrupt->function_regmap,
+				     interrupt->function, interrupt->entity,
+				     SDCA_CTL_XU_FDL_MESSAGEOFFSET, fdl_file->fdl_offset,
+				     SDCA_CTL_XU_FDL_MESSAGELENGTH, swf->data,
+				     swf->file_length - offsetof(struct acpi_sw_file, data));
+	release_firmware(firmware);
+	return ret;
+}
+
+static struct sdca_fdl_set *fdl_get_set(struct sdca_interrupt *interrupt)
+{
+	struct device *dev = interrupt->dev;
+	struct sdca_fdl_data *fdl_data = &interrupt->function->fdl_data;
+	struct sdca_entity *xu = interrupt->entity;
+	struct sdca_control_range *range;
+	unsigned int val;
+	int i, ret;
+
+	ret = regmap_read(interrupt->function_regmap,
+			  SDW_SDCA_CTL(interrupt->function->desc->adr, xu->id,
+				       SDCA_CTL_XU_FDL_SET_INDEX, 0),
+			  &val);
+	if (ret < 0) {
+		dev_err(dev, "failed to read FDL set index: %d\n", ret);
+		return NULL;
+	}
+
+	range = sdca_selector_find_range(dev, xu, SDCA_CTL_XU_FDL_SET_INDEX,
+					 SDCA_FDL_SET_INDEX_NCOLS, 0);
+
+	val = sdca_range_search(range, SDCA_FDL_SET_INDEX_SET_NUMBER,
+				val, SDCA_FDL_SET_INDEX_FILE_SET_ID);
+
+	for (i = 0; i < fdl_data->num_sets; i++) {
+		if (fdl_data->sets[i].id == val)
+			return &fdl_data->sets[i];
+	}
+
+	dev_err(dev, "invalid fileset id: %d\n", val);
+	return NULL;
+}
+
+static void fdl_end(struct sdca_interrupt *interrupt)
+{
+	struct fdl_state *fdl_state = interrupt->priv;
+
+	if (!fdl_state->set)
+		return;
+
+	fdl_state->set = NULL;
+
+	dev_dbg(interrupt->dev, "completed FDL process\n");
+}
+
+static unsigned int fdl_status_process(struct sdca_interrupt *interrupt,
+				       unsigned int status)
+{
+	struct fdl_state *fdl_state = interrupt->priv;
+	int ret;
+
+	switch (status) {
+	case SDCA_CTL_XU_FDLD_NEEDS_SET:
+		dev_dbg(interrupt->dev, "starting FDL process...\n");
+
+		fdl_state->file_index = 0;
+		fdl_state->set = fdl_get_set(interrupt);
+		fallthrough;
+	case SDCA_CTL_XU_FDLD_MORE_FILES_OK:
+		ret = fdl_load_file(interrupt, fdl_state->set, fdl_state->file_index);
+		if (ret) {
+			fdl_end(interrupt);
+			return SDCA_CTL_XU_FDLH_REQ_ABORT;
+		}
+
+		return SDCA_CTL_XU_FDLH_FILE_AVAILABLE;
+	case SDCA_CTL_XU_FDLD_FILE_OK:
+		if (!fdl_state->set) {
+			fdl_end(interrupt);
+			return SDCA_CTL_XU_FDLH_REQ_ABORT;
+		}
+
+		fdl_state->file_index++;
+
+		if (fdl_state->file_index < fdl_state->set->num_files)
+			return SDCA_CTL_XU_FDLH_MORE_FILES;
+		fallthrough;
+	case SDCA_CTL_XU_FDLD_COMPLETE:
+		fdl_end(interrupt);
+		return SDCA_CTL_XU_FDLH_COMPLETE;
+	default:
+		fdl_end(interrupt);
+
+		if (status & SDCA_CTL_XU_FDLD_REQ_RESET)
+			return SDCA_CTL_XU_FDLH_RESET_ACK;
+		else if (status & SDCA_CTL_XU_FDLD_REQ_ABORT)
+			return SDCA_CTL_XU_FDLH_COMPLETE;
+
+		dev_err(interrupt->dev, "invalid FDL status: %x\n", status);
+		return SDCA_CTL_XU_FDLH_REQ_ABORT;
+	}
+}
+
+/**
+ * sdca_fdl_process - Process the FDL state machine
+ * @interrupt: SDCA interrupt structure
+ *
+ * Based on section 13.2.5 Flow Diagram for File Download, Host side.
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_fdl_process(struct sdca_interrupt *interrupt)
+{
+	struct device *dev = interrupt->dev;
+	unsigned int reg, status, response;
+	int ret;
+
+	ret = sdca_ump_get_owner_host(dev, interrupt->function_regmap,
+				      interrupt->function, interrupt->entity,
+				      interrupt->control);
+	if (ret)
+		return ret;
+
+	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
+			   SDCA_CTL_XU_FDL_STATUS, 0);
+	ret = regmap_read(interrupt->function_regmap, reg, &status);
+	if (ret < 0) {
+		dev_err(dev, "failed to read FDL status: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(dev, "FDL status: %#x\n", status);
+
+	response = fdl_status_process(interrupt, status);
+
+	dev_dbg(dev, "FDL response: %#x\n", response);
+
+	ret = regmap_write(interrupt->function_regmap, reg,
+			   response | (status & ~SDCA_CTL_XU_FDLH_MASK));
+	if (ret < 0) {
+		dev_err(dev, "failed to set FDL status signal: %d\n", ret);
+		return ret;
+	}
+
+	ret = sdca_ump_set_owner_device(dev, interrupt->function_regmap,
+					interrupt->function, interrupt->entity,
+					interrupt->control);
+	if (ret)
+		return ret;
+
+	if (response == SDCA_CTL_XU_FDLH_RESET_ACK) {
+		dev_dbg(dev, "FDL request reset\n");
+		// FIXME: Add reset of device
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_fdl_process, "SND_SOC_SDCA");
-- 
2.47.2

[PATCH 12/15] ASoC: SDCA: Add XU-specific IRQ processing

[Charles Keepax]

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

Hookup the XU IRQs required for the FDL process.

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_interrupts.c | 34 ++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index a0677911503fb..c199b5a4f0d2e 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -18,8 +18,10 @@
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_registers.h>
 #include <sound/sdca.h>
+#include <sound/sdca_entity_xu.h>
 #include <sound/sdca_function.h>
 #include <sound/sdca_interrupts.h>
+#include <sound/sdca_ump.h>
 #include <sound/soc-component.h>
 #include <sound/soc.h>
 
@@ -245,6 +247,29 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	return irqret;
 }
 
+static irqreturn_t fdl_owner_handler(int irq, void *data)
+{
+	struct sdca_interrupt *interrupt = data;
+	struct device *dev = interrupt->dev;
+	irqreturn_t irqret = IRQ_NONE;
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to resume for fdl: %d\n", ret);
+		goto error;
+	}
+
+	ret = sdca_fdl_process(interrupt);
+	if (ret)
+		goto error;
+
+	irqret = IRQ_HANDLED;
+error:
+	pm_runtime_put(dev);
+	return irqret;
+}
+
 static int sdca_irq_request_locked(struct device *dev,
 				   struct sdca_interrupt_info *info,
 				   int sdca_irq, const char *name,
@@ -423,6 +448,15 @@ int sdca_irq_populate(struct sdca_function_data *function,
 				if (control->sel == SDCA_CTL_GE_DETECTED_MODE)
 					handler = detected_mode_handler;
 				break;
+			case SDCA_ENTITY_TYPE_XU:
+				if (control->sel == SDCA_CTL_XU_FDL_CURRENTOWNER) {
+					ret = sdca_fdl_alloc_state(interrupt);
+					if (ret)
+						return ret;
+
+					handler = fdl_owner_handler;
+				}
+				break;
 			default:
 				break;
 			}
-- 
2.47.2

[PATCH 13/15] ASoC: SDCA: Add completion for FDL start and stop

[Charles Keepax]

Add some completions and a helper function to allow other parts of the
system to wait for FDL to complete. The sdca_fdl_sync() function will
wait until it completes a full time out without a new FDL request
happening, this ensures that even parts requiring multiple rounds of FDL
should be fully downloaded before the driver boot continues.

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

diff --git a/include/sound/sdca_entity_xu.h b/include/sound/sdca_entity_xu.h
index 6f2ab0fc905a8..b3fb042cf9ba9 100644
--- a/include/sound/sdca_entity_xu.h
+++ b/include/sound/sdca_entity_xu.h
@@ -10,15 +10,24 @@
 #ifndef __SDCA_ENTITY_XU_H__
 #define __SDCA_ENTITY_XU_H__
 
+#include <linux/completion.h>
+
 struct sdca_fdl_set;
+struct sdca_function_data;
 struct sdca_interrupt;
+struct sdca_interrupt_info;
 
 /**
  * struct fdl_state - FDL state structure to keep data between interrupts
+ * @begin: Completion indicating the start of an FDL download cycle.
+ * @done: Completion indicating the end of an FDL download cycle.
  * @set: Pointer to the FDL set currently being downloaded.
  * @file_index: Index of the current file being processed.
  */
 struct fdl_state {
+	struct completion begin;
+	struct completion done;
+
 	struct sdca_fdl_set *set;
 	int file_index;
 };
@@ -48,5 +57,7 @@ struct fdl_state {
 
 int sdca_fdl_alloc_state(struct sdca_interrupt *interrupt);
 int sdca_fdl_process(struct sdca_interrupt *interrupt);
+int sdca_fdl_sync(struct sdca_function_data *function,
+		  struct sdca_interrupt_info *info);
 
 #endif // __SDCA_ENTITY_XU_H__
diff --git a/sound/soc/sdca/sdca_entity_xu.c b/sound/soc/sdca/sdca_entity_xu.c
index ff7758460edc3..383492d8238a1 100644
--- a/sound/soc/sdca/sdca_entity_xu.c
+++ b/sound/soc/sdca/sdca_entity_xu.c
@@ -14,6 +14,7 @@
 #include <linux/firmware.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/sprintf.h>
 #include <linux/soundwire/sdw.h>
@@ -39,12 +40,74 @@ int sdca_fdl_alloc_state(struct sdca_interrupt *interrupt)
 	if (!fdl_state)
 		return -ENOMEM;
 
+	init_completion(&fdl_state->begin);
+	init_completion(&fdl_state->done);
+
 	interrupt->priv = fdl_state;
 
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(sdca_fdl_alloc_state, "SND_SOC_SDCA");
 
+/**
+ * sdca_fdl_sync - wait for a function to finish FDL
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_fdl_sync(struct sdca_function_data *function,
+		  struct sdca_interrupt_info *info)
+{
+	unsigned long timeout = msecs_to_jiffies(1000);
+	int nfdl;
+	int i;
+
+	do {
+		nfdl = 0;
+
+		for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
+			struct sdca_interrupt *interrupt = &info->irqs[i];
+			struct device *dev = interrupt->dev;
+			struct fdl_state *fdl_state;
+			unsigned long time;
+
+			if (interrupt->function != function ||
+			    !interrupt->entity || !interrupt->control ||
+			    interrupt->entity->type != SDCA_ENTITY_TYPE_XU ||
+			    interrupt->control->sel != SDCA_CTL_XU_FDL_CURRENTOWNER)
+				continue;
+
+			fdl_state = interrupt->priv;
+			nfdl++;
+
+			/*
+			 * Looking for timeout without any new FDL requests
+			 * to imply the device has completed initial
+			 * firmware setup. Alas the specification doesn't
+			 * have any mechanism to detect this.
+			 */
+			time = wait_for_completion_timeout(&fdl_state->begin,
+							   timeout);
+			if (!time) {
+				dev_dbg(dev, "no new FDL starts\n");
+				nfdl--;
+				continue;
+			}
+
+			time = wait_for_completion_timeout(&fdl_state->done,
+							   timeout);
+			if (!time) {
+				dev_err(dev, "timed out waiting for FDL to complete\n");
+				return -ETIMEDOUT;
+			}
+		}
+	} while (nfdl);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_fdl_sync, "SND_SOC_SDCA");
+
 static char *fdl_get_sku_filename(struct device *dev,
 				  struct sdca_fdl_file *fdl_file)
 {
@@ -207,6 +270,9 @@ static void fdl_end(struct sdca_interrupt *interrupt)
 
 	fdl_state->set = NULL;
 
+	pm_runtime_put(interrupt->dev);
+	complete(&fdl_state->done);
+
 	dev_dbg(interrupt->dev, "completed FDL process\n");
 }
 
@@ -220,6 +286,9 @@ static unsigned int fdl_status_process(struct sdca_interrupt *interrupt,
 	case SDCA_CTL_XU_FDLD_NEEDS_SET:
 		dev_dbg(interrupt->dev, "starting FDL process...\n");
 
+		pm_runtime_get(interrupt->dev);
+		complete(&fdl_state->begin);
+
 		fdl_state->file_index = 0;
 		fdl_state->set = fdl_get_set(interrupt);
 		fallthrough;
-- 
2.47.2

[PATCH 14/15] ASoC: SDCA: Add early IRQ handling

[Charles Keepax]

Some IRQs (FDL) require processing before the primary soundcard is
brought up, as the downloaded files could be firmware required for
operation of the audio functions of the device. Add a new helper
function which registers the required IRQs.

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

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index 4c347fdce1658..d2843f88d783d 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -75,6 +75,9 @@ int sdca_irq_data_populate(struct device *dev, struct regmap *function_regmap,
 			   struct sdca_entity *entity,
 			   struct sdca_control *control,
 			   struct sdca_interrupt *interrupt);
+int sdca_irq_populate_early(struct device *dev, struct regmap *function_regmap,
+			    struct sdca_function_data *function,
+			    struct sdca_interrupt_info *info);
 int sdca_irq_populate(struct sdca_function_data *function,
 		      struct snd_soc_component *component,
 		      struct sdca_interrupt_info *info);
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index c199b5a4f0d2e..ed2a06a1b719f 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -396,6 +396,77 @@ static struct sdca_interrupt *get_interrupt_data(struct device *dev, int irq,
 	return &info->irqs[irq];
 }
 
+/**
+ * sdca_irq_populate_early - process pre-audio card IRQ registrations
+ * @dev: Device pointer for SDCA Function.
+ * @regmap: Regmap pointer for the SDCA Function.
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ *
+ * This is intended to be used as part of the Function boot process. It
+ * can be called before the soundcard is registered (ie. doesn't depend
+ * on component) and will register the FDL interrupts.
+ *
+ * Return: Zero on success, and a negative error code on failure.
+ */
+int sdca_irq_populate_early(struct device *dev, struct regmap *regmap,
+			    struct sdca_function_data *function,
+			    struct sdca_interrupt_info *info)
+{
+	int i, j;
+
+	guard(mutex)(&info->irq_lock);
+
+	for (i = 0; i < function->num_entities; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		for (j = 0; j < entity->num_controls; j++) {
+			struct sdca_control *control = &entity->controls[j];
+			int irq = control->interrupt_position;
+			struct sdca_interrupt *interrupt;
+			int ret;
+
+			interrupt = get_interrupt_data(dev, irq, info);
+			if (IS_ERR(interrupt))
+				return PTR_ERR(interrupt);
+			else if (!interrupt)
+				continue;
+
+			switch (entity->type) {
+			case SDCA_ENTITY_TYPE_XU:
+				if (control->sel != SDCA_CTL_XU_FDL_CURRENTOWNER)
+					break;
+
+				ret = sdca_irq_data_populate(dev, regmap, NULL,
+							     function, entity,
+							     control, interrupt);
+				if (ret)
+					return ret;
+
+				ret = sdca_fdl_alloc_state(interrupt);
+				if (ret)
+					return ret;
+
+				ret = sdca_irq_request_locked(dev, info, irq,
+							      interrupt->name,
+							      fdl_owner_handler,
+							      interrupt);
+				if (ret) {
+					dev_err(dev, "failed to request irq %s: %d\n",
+						interrupt->name, ret);
+					return ret;
+				}
+				break;
+			default:
+				break;
+			}
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_populate_early, "SND_SOC_SDCA");
+
 /**
  * sdca_irq_populate - Request all the individual IRQs for an SDCA Function
  * @function: Pointer to the SDCA Function.
-- 
2.47.2

[PATCH 15/15] ASoC: SDCA: Add HID button IRQ

[Charles Keepax]

Now full support for the UMP buffers is available, it is possible to
read the SDCA HID descriptors from the device and pass them to
user-space. Add a helper function to process HID events from an SDCA
device.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_hid.h         | 13 +++++++--
 sound/soc/sdca/sdca_hid.c        | 46 ++++++++++++++++++++++++++++++++
 sound/soc/sdca/sdca_interrupts.c | 28 +++++++++++++++++++
 3 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/include/sound/sdca_hid.h b/include/sound/sdca_hid.h
index 3a155835e035e..18bebbe428c9f 100644
--- a/include/sound/sdca_hid.h
+++ b/include/sound/sdca_hid.h
@@ -8,13 +8,17 @@
 #ifndef __SDCA_HID_H__
 #define __SDCA_HID_H__
 
-#include <linux/types.h>
-#include <linux/hid.h>
+struct device;
+struct sdw_slave;
+
+struct sdca_entity;
+struct sdca_interrupt;
 
 #if IS_ENABLED(CONFIG_SND_SOC_SDCA_HID)
 
 int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
 			struct sdca_entity *entity);
+int sdca_hid_process_report(struct sdca_interrupt *interrupt);
 
 #else
 
@@ -24,6 +28,11 @@ static inline int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
 	return 0;
 }
 
+static inline int sdca_hid_process_report(struct sdca_interrupt *interrupt)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* __SDCA_HID_H__ */
diff --git a/sound/soc/sdca/sdca_hid.c b/sound/soc/sdca/sdca_hid.c
index 53dad1a524d4b..ad53207b0d62b 100644
--- a/sound/soc/sdca/sdca_hid.c
+++ b/sound/soc/sdca/sdca_hid.c
@@ -10,6 +10,7 @@
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
+#include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/property.h>
 #include <linux/soundwire/sdw.h>
@@ -17,6 +18,8 @@
 #include <sound/sdca.h>
 #include <sound/sdca_function.h>
 #include <sound/sdca_hid.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/sdca_ump.h>
 
 static int sdwhid_parse(struct hid_device *hid)
 {
@@ -121,5 +124,48 @@ int sdca_add_hid_device(struct device *dev, struct sdw_slave *sdw,
 }
 EXPORT_SYMBOL_NS(sdca_add_hid_device, "SND_SOC_SDCA");
 
+/**
+ * sdca_hid_process_report - read a HID event from the device and report
+ * @interrupt: Pointer to the SDCA interrupt information structure.
+ *
+ * Return: Zero on success, and a negative error code on failure.
+ */
+int sdca_hid_process_report(struct sdca_interrupt *interrupt)
+{
+	struct device *dev = interrupt->dev;
+	struct hid_device *hid = interrupt->entity->hide.hid;
+	void *val __free(kfree) = NULL;
+	int len, ret;
+
+	ret = sdca_ump_get_owner_host(dev, interrupt->function_regmap,
+				      interrupt->function, interrupt->entity,
+				      interrupt->control);
+	if (ret)
+		return ret;
+
+	len = sdca_ump_read_message(dev, interrupt->device_regmap,
+				    interrupt->function_regmap,
+				    interrupt->function, interrupt->entity,
+				    SDCA_CTL_HIDE_HIDTX_MESSAGEOFFSET,
+				    SDCA_CTL_HIDE_HIDTX_MESSAGELENGTH, &val);
+	if (len < 0)
+		return len;
+
+	ret = sdca_ump_set_owner_device(dev, interrupt->function_regmap,
+					interrupt->function, interrupt->entity,
+					interrupt->control);
+	if (ret)
+		return ret;
+
+	ret = hid_input_report(hid, HID_INPUT_REPORT, val, len, true);
+	if (ret < 0) {
+		dev_err(dev, "failed to report hid event: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_hid_process_report, "SND_SOC_SDCA");
+
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_DESCRIPTION("SDCA HID library");
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index ed2a06a1b719f..386a15ebf1093 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -20,6 +20,7 @@
 #include <sound/sdca.h>
 #include <sound/sdca_entity_xu.h>
 #include <sound/sdca_function.h>
+#include <sound/sdca_hid.h>
 #include <sound/sdca_interrupts.h>
 #include <sound/sdca_ump.h>
 #include <sound/soc-component.h>
@@ -247,6 +248,29 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
 	return irqret;
 }
 
+static irqreturn_t hid_handler(int irq, void *data)
+{
+	struct sdca_interrupt *interrupt = data;
+	struct device *dev = interrupt->dev;
+	irqreturn_t irqret = IRQ_NONE;
+	int ret;
+
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to resume for hid: %d\n", ret);
+		goto error;
+	}
+
+	ret = sdca_hid_process_report(interrupt);
+	if (ret)
+		goto error;
+
+	irqret = IRQ_HANDLED;
+error:
+	pm_runtime_put(dev);
+	return irqret;
+}
+
 static irqreturn_t fdl_owner_handler(int irq, void *data)
 {
 	struct sdca_interrupt *interrupt = data;
@@ -528,6 +552,10 @@ int sdca_irq_populate(struct sdca_function_data *function,
 					handler = fdl_owner_handler;
 				}
 				break;
+			case SDCA_ENTITY_TYPE_HIDE:
+				if (control->sel == SDCA_CTL_HIDE_HIDTX_CURRENTOWNER)
+					handler = hid_handler;
+				break;
 			default:
 				break;
 			}
-- 
2.47.2

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Liao, Bard]

On 9/5/2025 10:31 PM, Charles Keepax wrote:
> In the future some IRQs (mostly File DownLoad) will need to run before
> the soundcard is constructed, as such refactor more of the IRQ handling
> to use raw device and regmap pointers rather than accessing things
> through the component.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---

> -int sdca_irq_data_populate(struct snd_soc_component *component,
> +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
> +			   struct snd_soc_component *component,
>  			   struct sdca_function_data *function,
>  			   struct sdca_entity *entity,
>  			   struct sdca_control *control,
>  			   struct sdca_interrupt *interrupt)
>  {
> -	struct device *dev = component->dev;

Previously, we assume 'component' will never be null.

>  	const char *name;
>  
> +	if (!dev && component)

But now we test 'component'. If we don't assume 'component' is not null
any more, we could test 'component' at the very beginning of this function.

> +		dev = component->dev;
> +	if (!dev)
> +		return -ENODEV;
> +
>  	name = devm_kasprintf(dev, GFP_KERNEL, "%s %s %s", function->desc->name,
>  			      entity->label, control->label);
>  	if (!name)
>  		return -ENOMEM;
>  
>  	interrupt->name = name;
> +	interrupt->dev = dev;
> +	if (!regmap && component)
> +		interrupt->function_regmap = component->regmap;
> +	else
> +		interrupt->function_regmap = regmap;
>  	interrupt->component = component;
>  	interrupt->function = function;
>  	interrupt->entity = entity;
> @@ -394,8 +406,9 @@ int sdca_irq_populate(struct sdca_function_data *function,
>  			else if (!interrupt)
>  				continue;
>  
> -			ret = sdca_irq_data_populate(component, function, entity,
> -						     control, interrupt);
> +			ret = sdca_irq_data_populate(dev, NULL, component,
> +						     function, entity, control,
> +						     interrupt);
>  			if (ret)
>  				return ret;
>  

Re: [PATCH 01/15] regmap: sdw-mbq: Don't assume the regmap device is the SoundWire slave

[Pierre-Louis Bossart]

On 9/5/25 16:31, Charles Keepax wrote:
> Currently, the code assumes that the device that registered the
> MBQ register map is the actual SoundWire slave device. This works
> fine for all current users, however future SDCA devices will
> likely be implemented with the SoundWire slave as a parent device
> and separate child drivers with regmaps for each audio Function.

I thought you wanted a single regmap shared by all function drivers?
Maybe it's the wording that throws me off but are you now leaning to have multiple regmaps, one per child device/driver?

Re: [PATCH 02/15] ASoC: SDCA: Add manual PM runtime gets to IRQ handlers

[Pierre-Louis Bossart]

On 9/5/25 16:31, Charles Keepax wrote:
> SDCA interrupt handling is a bit odd, the SDCA IRQ registers are
> defined on a device level but the handling of the IRQ is at the
> Function level. As such the regmap IRQ's PM runtime operations need to
> be on the device itself to ensure those registers are available but an
> additional runtime get is required for the Function child when the IRQ
> is actually handled. Add the required manual PM runtime gets.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  sound/soc/sdca/sdca_interrupts.c | 42 ++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
> index 79bf3042f57d4..6775f0f7b5659 100644
> --- a/sound/soc/sdca/sdca_interrupts.c
> +++ b/sound/soc/sdca/sdca_interrupts.c
> @@ -11,7 +11,9 @@
>  #include <linux/bits.h>
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
> +#include <linux/dev_printk.h>
>  #include <linux/interrupt.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_registers.h>
> @@ -86,18 +88,25 @@ static irqreturn_t function_status_handler(int irq, void *data)
>  {
>  	struct sdca_interrupt *interrupt = data;
>  	struct device *dev = interrupt->component->dev;
> +	irqreturn_t irqret = IRQ_NONE;
>  	unsigned int reg, val;
>  	unsigned long status;
>  	unsigned int mask;
>  	int ret;
>  
> +	ret = pm_runtime_get_sync(dev);

what does 'dev' represent? The SDCA function device or the parent SoundWire peripheral device?

I would think it's the former, with the parent-child relationship used by the runtime_pm framework to resume the parent peripheral device if needed.

maybe use sdev and pdev as an arbitrary convention to help the reader?

> +	if (ret < 0) {
> +		dev_err(dev, "failed to resume for function status: %d\n", ret);
> +		goto error;
> +	}
> +
>  	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
>  			   interrupt->control->sel, 0);
>  
>  	ret = regmap_read(interrupt->component->regmap, reg, &val);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to read function status: %d\n", ret);
> -		return IRQ_NONE;
> +		goto error;
>  	}
>  
>  	dev_dbg(dev, "function status: %#x\n", val);
> @@ -130,10 +139,13 @@ static irqreturn_t function_status_handler(int irq, void *data)
>  	ret = regmap_write(interrupt->component->regmap, reg, val);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to clear function status: %d\n", ret);
> -		return IRQ_NONE;
> +		goto error;
>  	}
>  
> -	return IRQ_HANDLED;
> +	irqret = IRQ_HANDLED;
> +error:
> +	pm_runtime_put(dev);
> +	return irqret;
>  }

Re: [PATCH 04/15] ASoC: SDCA: Pass device register map from IRQ alloc to handlers

[Pierre-Louis Bossart]

On 9/5/25 16:31, Charles Keepax wrote:
> Store a copy of the device register map in the structure for the IRQ
> handlers. This will allow the individual IRQ handlers access to the
> device level register map if required.

Sorry, not following how you plan on sharing access to regmap between parent and child devices.


> @@ -445,6 +445,9 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
>  
>  	info->irq_chip = sdca_irq_chip;
>  
> +	for (i = 0; i < ARRAY_SIZE(info->irqs); i++)
> +		info->irqs[i].device_regmap = regmap;
> +

Lost on this one as well. A child device could support multiple interrupts, e.g. for buttons and jack, not sure why each interrupt needs a separate pointer to the same identical information.

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Pierre-Louis Bossart]

On 9/5/25 16:31, Charles Keepax wrote:
> In the future some IRQs (mostly File DownLoad) will need to run before

nitpick: I think you meant UMP interrupts used e.g. for file download. I am not aware of any interrupts for file download proper.

> the soundcard is constructed, as such refactor more of the IRQ handling
> to use raw device and regmap pointers rather than accessing things
> through the component.

Maybe elaborate that these interrupts would be used as soon as the actual SoundWire device attaches on the link.

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

On 9/5/25 16:31, Charles Keepax wrote:
> Add helper functions for handling Universal Message Passing (UMP)
> buffers on SDCA devices. These are generic mechanisms to pass blocks of
> binary data between the host and the device, in both directions. They
> are used for things like passing HID descriptors and the File Download
> process.

Humm, these helpers are very host-centric and don't seem to cover the basic UMP message passing, e.g. for host to device:

1. host puts information in a buffer.
2. host sets ownership to device and waits for ownership to be returned.
3. device does a bunch of things and returns the ownership to the host.
4. host detects ownership change or times out with a failed transfer error message.

What am I missing?

> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  include/sound/sdca_function.h |  26 ++++
>  include/sound/sdca_ump.h      |  45 +++++++
>  sound/soc/sdca/Makefile       |   3 +-
>  sound/soc/sdca/sdca_ump.c     | 247 ++++++++++++++++++++++++++++++++++
>  4 files changed, 320 insertions(+), 1 deletion(-)
>  create mode 100644 include/sound/sdca_ump.h
>  create mode 100644 sound/soc/sdca/sdca_ump.c
> 
> diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
> index ab9af84082c95..341912d896679 100644
> --- a/include/sound/sdca_function.h
> +++ b/include/sound/sdca_function.h
> @@ -133,6 +133,32 @@ struct sdca_init_write {
>  #define SDCA_CTL_TYPE_S(ent, sel) SDCA_CTL_TYPE(SDCA_ENTITY_TYPE_##ent, \
>  						SDCA_CTL_##ent##_##sel)
>  
> +/**
> + * enum sdca_messageoffset_range - Column definitions UMP MessageOffset
> + */
> +enum sdca_messageoffset_range {
> +	SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS		= 0,
> +	SDCA_MESSAGEOFFSET_BUFFER_LENGTH		= 1,
> +	SDCA_MESSAGEOFFSET_UMP_MODE			= 2,
> +	SDCA_MESSAGEOFFSET_NCOLS			= 3,
> +};
> +
> +/**
> + * enum sdca_ump_mode - SDCA UMP Mode
> + */
> +enum sdca_ump_mode {
> +	SDCA_UMP_MODE_DIRECT				= 0x00,
> +	SDCA_UMP_MODE_INDIRECT				= 0x01,
> +};
> +
> +/**
> + * enum sdca_ump_owner - SDCA UMP Owner
> + */
> +enum sdca_ump_owner {
> +	SDCA_UMP_OWNER_HOST				= 0x00,
> +	SDCA_UMP_OWNER_DEVICE				= 0x01,
> +};
> +
>  /**
>   * enum sdca_it_controls - SDCA Controls for Input Terminal
>   *
> diff --git a/include/sound/sdca_ump.h b/include/sound/sdca_ump.h
> new file mode 100644
> index 0000000000000..b2363199d19aa
> --- /dev/null
> +++ b/include/sound/sdca_ump.h
> @@ -0,0 +1,45 @@
> +/* 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_UMP_H__
> +#define __SDCA_UMP_H__
> +
> +struct regmap;
> +struct sdca_control;
> +struct sdca_entity;
> +struct sdca_function_data;
> +struct snd_soc_component;
> +
> +int sdca_ump_get_owner_host(struct device *dev,
> +			    struct regmap *function_regmap,
> +			    struct sdca_function_data *function,
> +			    struct sdca_entity *entity,
> +			    struct sdca_control *control);
> +int sdca_ump_set_owner_device(struct device *dev,
> +			      struct regmap *function_regmap,
> +			      struct sdca_function_data *function,
> +			      struct sdca_entity *entity,
> +			      struct sdca_control *control);
> +int sdca_ump_read_message(struct device *dev,
> +			  struct regmap *device_regmap,
> +			  struct regmap *function_regmap,
> +			  struct sdca_function_data *function,
> +			  struct sdca_entity *entity,
> +			  unsigned int offset_sel, unsigned int length_sel,
> +			  void **msg);
> +int sdca_ump_write_message(struct device *dev,
> +			   struct regmap *device_regmap,
> +			   struct regmap *function_regmap,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   unsigned int offset_sel, unsigned int msg_offset,
> +			   unsigned int length_sel,
> +			   void *msg, int msg_len);
> +
> +#endif // __SDCA_UMP_H__
> diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
> index 5e51760cb6513..a1b24c95cd8c8 100644
> --- a/sound/soc/sdca/Makefile
> +++ b/sound/soc/sdca/Makefile
> @@ -1,6 +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
> +snd-soc-sdca-y := sdca_functions.o sdca_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
>  
> diff --git a/sound/soc/sdca/sdca_ump.c b/sound/soc/sdca/sdca_ump.c
> new file mode 100644
> index 0000000000000..5dcad2f7ea05b
> --- /dev/null
> +++ b/sound/soc/sdca/sdca_ump.c
> @@ -0,0 +1,247 @@
> +// 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/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <sound/sdca.h>
> +#include <sound/sdca_function.h>
> +#include <sound/sdca_ump.h>
> +#include <sound/soc-component.h>
> +#include <linux/soundwire/sdw_registers.h>
> +
> +/**
> + * sdca_ump_get_owner_host - check a UMP buffer is owned by the host
> + * @dev: Pointer to the struct device used for error messages.
> + * @function_regmap: Pointer to the regmap for the SDCA Function.
> + * @function: Pointer to the Function information.
> + * @entity: Pointer to the SDCA Entity.
> + * @control: Pointer to the SDCA Control for the UMP Owner.
> + *
> + * Return: Returns zero on success, and a negative error code on failure.
> + */
> +int sdca_ump_get_owner_host(struct device *dev,
> +			    struct regmap *function_regmap,
> +			    struct sdca_function_data *function,
> +			    struct sdca_entity *entity,
> +			    struct sdca_control *control)
> +{
> +	unsigned int reg, owner;
> +	int ret;
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
> +	ret = regmap_read(function_regmap, reg, &owner);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to read UMP owner: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	if (owner != SDCA_UMP_OWNER_HOST) {
> +		dev_err(dev, "%s: host is not the UMP owner\n", entity->label);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_ump_get_owner_host, "SND_SOC_SDCA");
> +
> +/**
> + * sdca_ump_set_owner_device - set a UMP buffer's ownership back to the device
> + * @dev: Pointer to the struct device used for error messages.
> + * @function_regmap: Pointer to the regmap for the SDCA Function.
> + * @function: Pointer to the Function information.
> + * @entity: Pointer to the SDCA Entity.
> + * @control: Pointer to the SDCA Control for the UMP Owner.
> + *
> + * Return: Returns zero on success, and a negative error code on failure.
> + */
> +int sdca_ump_set_owner_device(struct device *dev,
> +			      struct regmap *function_regmap,
> +			      struct sdca_function_data *function,
> +			      struct sdca_entity *entity,
> +			      struct sdca_control *control)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
> +	ret = regmap_write(function_regmap, reg, SDCA_UMP_OWNER_DEVICE);
> +	if (ret < 0)
> +		dev_err(dev, "%s: failed to write UMP owner: %d\n",
> +			entity->label, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_ump_set_owner_device, "SND_SOC_SDCA");
> +
> +/**
> + * sdca_ump_read_message - read a UMP message from the device
> + * @dev: Pointer to the struct device used for error messages.
> + * @device_regmap: Pointer to the Device register map.
> + * @function_regmap: Pointer to the regmap for the SDCA Function.
> + * @function: Pointer to the Function information.
> + * @entity: Pointer to the SDCA Entity.
> + * @offset_sel: Control Selector for the UMP Offset Control.
> + * @length_sel: Control Selector for the UMP Length Control.
> + * @msg: Pointer that will be populated with an dynamically buffer
> + * containing the UMP message. Note this needs to be freed by the
> + * caller.
> + *
> + * The caller should first call sdca_ump_get_owner_host() to ensure the host
> + * currently owns the UMP buffer, and then this function can be used to
> + * retrieve a message. It is the callers responsibility to free the
> + * message once it is finished with it. Finally sdca_ump_set_owner_device()
> + * should be called to return the buffer to the device.
> + *
> + * Return: Returns the message length on success, and a negative error
> + * code on failure.
> + */
> +int sdca_ump_read_message(struct device *dev,
> +			  struct regmap *device_regmap,
> +			  struct regmap *function_regmap,
> +			  struct sdca_function_data *function,
> +			  struct sdca_entity *entity,
> +			  unsigned int offset_sel, unsigned int length_sel,
> +			  void **msg)
> +{
> +	struct sdca_control_range *range;
> +	unsigned int msg_offset, msg_len;
> +	unsigned int buf_addr, buf_len;
> +	unsigned int reg;
> +	int ret;
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, offset_sel, 0);
> +	ret = regmap_read(function_regmap, reg, &msg_offset);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to read UMP offset: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	range = sdca_selector_find_range(dev, entity, offset_sel,
> +					 SDCA_MESSAGEOFFSET_NCOLS, 1);
> +	if (!range)
> +		return -ENOENT;
> +
> +	buf_addr = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS, 0);
> +	buf_len = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_LENGTH, 0);
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, length_sel, 0);
> +	ret = regmap_read(function_regmap, reg, &msg_len);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to read UMP length: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	if (msg_len > buf_len - msg_offset) {
> +		dev_err(dev, "%s: message too big for UMP buffer: %d\n",
> +			entity->label, msg_len);
> +		return -EINVAL;
> +	}
> +
> +	*msg = kmalloc(msg_len, GFP_KERNEL);
> +	if (!*msg)
> +		return -ENOMEM;
> +
> +	ret = regmap_raw_read(device_regmap, buf_addr + msg_offset, *msg, msg_len);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to read UMP message: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	return msg_len;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_ump_read_message, "SND_SOC_SDCA");
> +
> +/**
> + * sdca_ump_write_message - write a UMP message to the device
> + * @dev: Pointer to the struct device used for error messages.
> + * @device_regmap: Pointer to the Device register map.
> + * @function_regmap: Pointer to the regmap for the SDCA Function.
> + * @function: Pointer to the Function information.
> + * @entity: Pointer to the SDCA Entity.
> + * @offset_sel: Control Selector for the UMP Offset Control.
> + * @msg_offset: Offset within the UMP buffer at which the message should
> + * be written.
> + * @length_sel: Control Selector for the UMP Length Control.
> + * @msg: Pointer to the data that should be written to the UMP buffer.
> + * @msg_len: Length of the message data in bytes.
> + *
> + * The caller should first call sdca_ump_get_owner_host() to ensure the host
> + * currently owns the UMP buffer, and then this function can be used to
> + * write a message. Finally sdca_ump_set_owner_device() should be called to
> + * return the buffer to the device, allowing the device to access the
> + * message.
> + *
> + * Return: Returns zero on success, and a negative error code on failure.
> + */
> +int sdca_ump_write_message(struct device *dev,
> +			   struct regmap *device_regmap,
> +			   struct regmap *function_regmap,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   unsigned int offset_sel, unsigned int msg_offset,
> +			   unsigned int length_sel,
> +			   void *msg, int msg_len)
> +{
> +	struct sdca_control_range *range;
> +	unsigned int buf_addr, buf_len, ump_mode;
> +	unsigned int reg;
> +	int ret;
> +
> +	range = sdca_selector_find_range(dev, entity, offset_sel,
> +					 SDCA_MESSAGEOFFSET_NCOLS, 1);
> +	if (!range)
> +		return -ENOENT;
> +
> +	buf_addr = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_START_ADDRESS, 0);
> +	buf_len = sdca_range(range, SDCA_MESSAGEOFFSET_BUFFER_LENGTH, 0);
> +	ump_mode = sdca_range(range, SDCA_MESSAGEOFFSET_UMP_MODE, 0);
> +
> +	if (msg_len > buf_len - msg_offset) {
> +		dev_err(dev, "%s: message too big for UMP buffer: %d\n",
> +			entity->label, msg_len);
> +		return -EINVAL;
> +	}
> +
> +	if (ump_mode != SDCA_UMP_MODE_DIRECT) {
> +		dev_err(dev, "%s: only direct mode currently supported\n",
> +			entity->label);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_raw_write(device_regmap, buf_addr + msg_offset, msg, msg_len);
> +	if (ret) {
> +		dev_err(dev, "%s: failed to write UMP message: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, offset_sel, 0);
> +	ret = regmap_write(function_regmap, reg, msg_offset);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to write UMP offset: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	reg = SDW_SDCA_CTL(function->desc->adr, entity->id, length_sel, 0);
> +	ret = regmap_write(function_regmap, reg, msg_len);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to write UMP length: %d\n",
> +			entity->label, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_ump_write_message, "SND_SOC_SDCA");

Re: [PATCH 11/15] ASoC: SDCA: Add FDL library for XU entities

[Pierre-Louis Bossart]

 
> +config SND_SOC_SDCA_XU
> +	bool "SDCA XU support"
> +	depends on SND_SOC_SDCA
> +	default y
> +	help
> +	  This option enables support for the MIPI SDCA XU Entity driver.
> +

I completely agree on the need for a common library for file download. But that functionality happens to be part of the XU scope. Other things might be added by vendors for secret-sauce controls.

IOW, this isn't XU support per se but FDL support only. An actual vendor-specific XU driver would rely on the FDL library and add more things of top.

Re: [PATCH 01/15] regmap: sdw-mbq: Don't assume the regmap device is the SoundWire slave

[Charles Keepax]

On Mon, Sep 08, 2025 at 01:35:44PM +0200, Pierre-Louis Bossart wrote:
> On 9/5/25 16:31, Charles Keepax wrote:
> > Currently, the code assumes that the device that registered the
> > MBQ register map is the actual SoundWire slave device. This works
> > fine for all current users, however future SDCA devices will
> > likely be implemented with the SoundWire slave as a parent device
> > and separate child drivers with regmaps for each audio Function.
> 
> I thought you wanted a single regmap shared by all function drivers?
> Maybe it's the wording that throws me off but are you now leaning to
> have multiple regmaps, one per child device/driver?

Yeah I think you convinced us about the regmap per function
thing, so that is the current plan.

Thanks,
Charles

Re: [PATCH 04/15] ASoC: SDCA: Pass device register map from IRQ alloc to handlers

[Charles Keepax]

On Mon, Sep 08, 2025 at 01:49:43PM +0200, Pierre-Louis Bossart wrote:
> On 9/5/25 16:31, Charles Keepax wrote:
> > Store a copy of the device register map in the structure for the IRQ
> > handlers. This will allow the individual IRQ handlers access to the
> > device level register map if required.
> 
> Sorry, not following how you plan on sharing access to regmap between
> parent and child devices.

They all have a pointer to the device level register map, if some
function or the device level is using it the regmap lock will be
held and the others are block.

> 
> 
> > @@ -445,6 +445,9 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
> >  
> >  	info->irq_chip = sdca_irq_chip;
> >  
> > +	for (i = 0; i < ARRAY_SIZE(info->irqs); i++)
> > +		info->irqs[i].device_regmap = regmap;
> > +
> 
> Lost on this one as well. A child device could support multiple
> interrupts, e.g. for buttons and jack, not sure why each interrupt
> needs a separate pointer to the same identical information.

Most IRQs don't need custom handling so the core can handle those
IRQs. In the case of custom IRQs the driver would register some
private data structure that would contain necessary information
for the handler such as regmaps. This array of interrupt
structures is basically that private data. A pointer to this
structure will be passed to the IRQ when it fires and it needs to
contain all the stuff the IRQ handler needs, one of those things
might be the device level register map.

Thanks,
Charles

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Charles Keepax]

On Mon, Sep 08, 2025 at 06:27:40PM +0800, Liao, Bard wrote:
> 
> 
> On 9/5/2025 10:31 PM, Charles Keepax wrote:
> > In the future some IRQs (mostly File DownLoad) will need to run before
> > the soundcard is constructed, as such refactor more of the IRQ handling
> > to use raw device and regmap pointers rather than accessing things
> > through the component.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> 
> > -int sdca_irq_data_populate(struct snd_soc_component *component,
> > +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
> > +			   struct snd_soc_component *component,
> >  			   struct sdca_function_data *function,
> >  			   struct sdca_entity *entity,
> >  			   struct sdca_control *control,
> >  			   struct sdca_interrupt *interrupt)
> >  {
> > -	struct device *dev = component->dev;
> 
> Previously, we assume 'component' will never be null.
> 
> >  	const char *name;
> >  
> > +	if (!dev && component)
> 
> But now we test 'component'. If we don't assume 'component' is not null
> any more, we could test 'component' at the very beginning of this function.
> 

Good spot, that was missed when moving from an older version of
the code. I will get that fixed up.

Thanks,
Charles

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Charles Keepax]

On Mon, Sep 08, 2025 at 01:55:21PM +0200, Pierre-Louis Bossart wrote:
> On 9/5/25 16:31, Charles Keepax wrote:
> > In the future some IRQs (mostly File DownLoad) will need to run before
> 
> nitpick: I think you meant UMP interrupts used e.g. for file
> download. I am not aware of any interrupts for file download
> proper.
> 
> > the soundcard is constructed, as such refactor more of the IRQ handling
> > to use raw device and regmap pointers rather than accessing things
> > through the component.
> 
> Maybe elaborate that these interrupts would be used as soon
> as the actual SoundWire device attaches on the link.

Certainly no object to tweaking the wording on both of those.

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
> On 9/5/25 16:31, Charles Keepax wrote:
> > Add helper functions for handling Universal Message Passing (UMP)
> > buffers on SDCA devices. These are generic mechanisms to pass blocks of
> > binary data between the host and the device, in both directions. They
> > are used for things like passing HID descriptors and the File Download
> > process.
> 
> Humm, these helpers are very host-centric and don't seem to cover the basic
> UMP message passing, e.g. for host to device:
> 
> 1. host puts information in a buffer.

by calling sdca_ump_write_message

> 2. host sets ownership to device and waits for ownership to be returned.

by calling sdca_ump_set_owner_device

> 3. device does a bunch of things and returns the ownership to the host.
> 4. host detects ownership change or times out with a failed transfer error message.

by receiving an IRQ on the owner control.

> What am I missing?

You might need to elaborate a little more on what you think is
missing. The four functions implement the four operations that
power a UMP buffer, checking you have ownership, passing
ownership back to the device, reading data, and writing data.

One could combine more of the functionality into a single helper,
but then you start to hit problems where you need to do extra
bits.

For example HID, very simple just: check you own the buffer,
read the message, and pass the buffer back.

FDL, has extra steps: check you own the buffer, check the FDL
status, then write the buffer, then write the FDL response,
and only then pass the buffer back.

Thanks,
Charles

Re: [PATCH 11/15] ASoC: SDCA: Add FDL library for XU entities

[Charles Keepax]

On Mon, Sep 08, 2025 at 02:14:00PM +0200, Pierre-Louis Bossart wrote:
>  
> > +config SND_SOC_SDCA_XU
> > +	bool "SDCA XU support"
> > +	depends on SND_SOC_SDCA
> > +	default y
> > +	help
> > +	  This option enables support for the MIPI SDCA XU Entity driver.
> > +
> 
> I completely agree on the need for a common library for file
> download. But that functionality happens to be part of the XU
> scope. Other things might be added by vendors for secret-sauce
> controls.
> 
> IOW, this isn't XU support per se but FDL support only. An
> actual vendor-specific XU driver would rely on the FDL library
> and add more things of top.

We can rename this to be more directly FDL, that seems fine.

Thanks,
Charles

Re: [PATCH 02/15] ASoC: SDCA: Add manual PM runtime gets to IRQ handlers

[Charles Keepax]

On Mon, Sep 08, 2025 at 01:42:40PM +0200, Pierre-Louis Bossart wrote:
> On 9/5/25 16:31, Charles Keepax wrote:
> > @@ -86,18 +88,25 @@ static irqreturn_t function_status_handler(int irq, void *data)
> >  {
> >  	struct sdca_interrupt *interrupt = data;
> >  	struct device *dev = interrupt->component->dev;
> > +	irqreturn_t irqret = IRQ_NONE;
> >  	unsigned int reg, val;
> >  	unsigned long status;
> >  	unsigned int mask;
> >  	int ret;
> >  
> > +	ret = pm_runtime_get_sync(dev);
> 
> what does 'dev' represent? The SDCA function device or the
> parent SoundWire peripheral device?

SDCA function device here.

> I would think it's the former, with the parent-child
> relationship used by the runtime_pm framework to resume the
> parent peripheral device if needed.

Yes this would cause the parent to power up, but there is
nuance here. The parent has actually already been powered up
by regmap IRQ. The thing here is SDCA is weird, the interrupts
are defined at the device level, but all the handling is at the
function level.

To some extent this is also a consequence of having a regmap per
function. Regmap IRQ is registered against the SoundWire device,
as it needs to access the device level IRQ registers. But the
handlers are per function and need to make sure an individual
functions register map is out of cache_only so need to do a
runtime get on the function specifically. In the past with
multi-function codecs we have been able to cheat a little here
as resuming the parent actually did everything necessary to
communicate with a part of the device.

> maybe use sdev and pdev as an arbitrary convention to help the reader?
> 

I quite like the idea of this as a naming convention, will have
a look at updating stuff.

Thanks,
Charles

Re: [PATCH 02/15] ASoC: SDCA: Add manual PM runtime gets to IRQ handlers

[Charles Keepax]

On Mon, Sep 08, 2025 at 02:31:01PM +0100, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 01:42:40PM +0200, Pierre-Louis Bossart wrote:
> > On 9/5/25 16:31, Charles Keepax wrote:
> > maybe use sdev and pdev as an arbitrary convention to help the reader?
> 
> I quite like the idea of this as a naming convention, will have
> a look at updating stuff.

Ok going through the code, basically almost everything uses the
function device, so I think what makes the most sense from a
churn point of view is: dev=function device, sdev=parent/soundwire
device. So I will update things to that.

Thanks,
Charles

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Charles Keepax]

On Mon, Sep 08, 2025 at 01:56:57PM +0100, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 06:27:40PM +0800, Liao, Bard wrote:
> > 
> > 
> > On 9/5/2025 10:31 PM, Charles Keepax wrote:
> > > In the future some IRQs (mostly File DownLoad) will need to run before
> > > the soundcard is constructed, as such refactor more of the IRQ handling
> > > to use raw device and regmap pointers rather than accessing things
> > > through the component.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > ---
> > 
> > > -int sdca_irq_data_populate(struct snd_soc_component *component,
> > > +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
> > > +			   struct snd_soc_component *component,
> > >  			   struct sdca_function_data *function,
> > >  			   struct sdca_entity *entity,
> > >  			   struct sdca_control *control,
> > >  			   struct sdca_interrupt *interrupt)
> > >  {
> > > -	struct device *dev = component->dev;
> > 
> > Previously, we assume 'component' will never be null.
> > 
> > >  	const char *name;
> > >  
> > > +	if (!dev && component)
> > 
> > But now we test 'component'. If we don't assume 'component' is not null
> > any more, we could test 'component' at the very beginning of this function.
> 
> Good spot, that was missed when moving from an older version of
> the code. I will get that fixed up.

Apologies I had slightly misinterpretted this, and thought you
meant we were dereferencing component before testing it in
sdca_irq_data_populate, but we arn't.

The intention of this patch is to allow the user to call
scda_irq_data_populate with or without a component. If you call
it with a component and pass NULL for dev/regmap it will use the
component for those. If you pass dev/regmap, it is fine to pass
NULL for component, the current user doesn't do this but the user
added later in the series will.

Thanks,
Charles

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Liao, Bard]

On 9/8/2025 10:43 PM, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 01:56:57PM +0100, Charles Keepax wrote:
>> On Mon, Sep 08, 2025 at 06:27:40PM +0800, Liao, Bard wrote:
>>>
>>>
>>> On 9/5/2025 10:31 PM, Charles Keepax wrote:
>>>> In the future some IRQs (mostly File DownLoad) will need to run before
>>>> the soundcard is constructed, as such refactor more of the IRQ handling
>>>> to use raw device and regmap pointers rather than accessing things
>>>> through the component.
>>>>
>>>> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>>>> ---
>>>
>>>> -int sdca_irq_data_populate(struct snd_soc_component *component,
>>>> +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
>>>> +			   struct snd_soc_component *component,
>>>>  			   struct sdca_function_data *function,
>>>>  			   struct sdca_entity *entity,
>>>>  			   struct sdca_control *control,
>>>>  			   struct sdca_interrupt *interrupt)
>>>>  {
>>>> -	struct device *dev = component->dev;
>>>
>>> Previously, we assume 'component' will never be null.
>>>
>>>>  	const char *name;
>>>>  
>>>> +	if (!dev && component)
>>>
>>> But now we test 'component'. If we don't assume 'component' is not null
>>> any more, we could test 'component' at the very beginning of this function.
>>
>> Good spot, that was missed when moving from an older version of
>> the code. I will get that fixed up.
> 
> Apologies I had slightly misinterpretted this, and thought you
> meant we were dereferencing component before testing it in
> sdca_irq_data_populate, but we arn't.

No, my question is about whether component can be NULL.

> 
> The intention of this patch is to allow the user to call
> scda_irq_data_populate with or without a component. If you call
> it with a component and pass NULL for dev/regmap it will use the
> component for those. If you pass dev/regmap, it is fine to pass
> NULL for component, the current user doesn't do this but the user
> added later in the series will.

Understood. I had the question because the change below.

+	interrupt->dev = dev;
+	if (!regmap && component)
+		interrupt->function_regmap = component->regmap;
+	else
+		interrupt->function_regmap = regmap;

If both regmap and component are null, the interrupt->function_regmap
will be NULL. Is it valid?

> 
> Thanks,
> Charles
> 

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Charles Keepax]

On Tue, Sep 09, 2025 at 09:42:20AM +0800, Liao, Bard wrote:
> On 9/8/2025 10:43 PM, Charles Keepax wrote:
> > On Mon, Sep 08, 2025 at 01:56:57PM +0100, Charles Keepax wrote:
> >> On Mon, Sep 08, 2025 at 06:27:40PM +0800, Liao, Bard wrote:
> >>> On 9/5/2025 10:31 PM, Charles Keepax wrote:
> >>>> -int sdca_irq_data_populate(struct snd_soc_component *component,
> >>>> +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
> >>>> +			   struct snd_soc_component *component,
> >>>>  			   struct sdca_function_data *function,
> >>>>  			   struct sdca_entity *entity,
> >>>>  			   struct sdca_control *control,
> >>>>  			   struct sdca_interrupt *interrupt)
> >>>>  {
> >>>> -	struct device *dev = component->dev;
> >>>
> >>> Previously, we assume 'component' will never be null.
> >>>
> >>>>  	const char *name;
> >>>>  
> >>>> +	if (!dev && component)
> >>>
> >>> But now we test 'component'. If we don't assume 'component' is not null
> >>> any more, we could test 'component' at the very beginning of this function.
> >>
> >> Good spot, that was missed when moving from an older version of
> >> the code. I will get that fixed up.
> > 
> > Apologies I had slightly misinterpretted this, and thought you
> > meant we were dereferencing component before testing it in
> > sdca_irq_data_populate, but we arn't.
> 
> No, my question is about whether component can be NULL.

The expectation would be component can not be NULL in
sdca_irq_populate(), but it could be NULL inside
sdca_irq_data_populate(), if called from somewhere else.

> Understood. I had the question because the change below.
> 
> +	interrupt->dev = dev;
> +	if (!regmap && component)
> +		interrupt->function_regmap = component->regmap;
> +	else
> +		interrupt->function_regmap = regmap;
> 
> If both regmap and component are null, the interrupt->function_regmap
> will be NULL. Is it valid?

Ok... yeah I see. I guess it depends on the IRQ handler that
ultimately gets registered. In most cases I would expect the
handler to need a regmap, however, we don't really validate
any of the data that is purely for the handler. For example we
don't validate function/entity/control either, we check dev for
NULL because it is used for devm allocation of the name string,
not because it is data for the handler. I think doing it this
way saves us making assumptions, once we start doing so we start
to constraint what can be done through the API.

Thanks,
Charles

Re: [PATCH 02/15] ASoC: SDCA: Add manual PM runtime gets to IRQ handlers

[Pierre-Louis Bossart]

On 9/8/25 15:31, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 01:42:40PM +0200, Pierre-Louis Bossart wrote:
>> On 9/5/25 16:31, Charles Keepax wrote:
>>> @@ -86,18 +88,25 @@ static irqreturn_t function_status_handler(int irq, void *data)
>>>  {
>>>  	struct sdca_interrupt *interrupt = data;
>>>  	struct device *dev = interrupt->component->dev;
>>> +	irqreturn_t irqret = IRQ_NONE;
>>>  	unsigned int reg, val;
>>>  	unsigned long status;
>>>  	unsigned int mask;
>>>  	int ret;
>>>  
>>> +	ret = pm_runtime_get_sync(dev);
>>
>> what does 'dev' represent? The SDCA function device or the
>> parent SoundWire peripheral device?
> 
> SDCA function device here.
> 
>> I would think it's the former, with the parent-child
>> relationship used by the runtime_pm framework to resume the
>> parent peripheral device if needed.
> 
> Yes this would cause the parent to power up, but there is
> nuance here. The parent has actually already been powered up
> by regmap IRQ. The thing here is SDCA is weird, the interrupts
> are defined at the device level, but all the handling is at the
> function level.
> 
> To some extent this is also a consequence of having a regmap per
> function. Regmap IRQ is registered against the SoundWire device,
> as it needs to access the device level IRQ registers. But the
> handlers are per function and need to make sure an individual
> functions register map is out of cache_only so need to do a
> runtime get on the function specifically. In the past with
> multi-function codecs we have been able to cheat a little here
> as resuming the parent actually did everything necessary to
> communicate with a part of the device.

ok, makes sense.

Capturing some of this paragraph in the commit message would be good to explain what resume operations mean for an SDCA child device. 
>> maybe use sdev and pdev as an arbitrary convention to help the reader?
>>
> 
> I quite like the idea of this as a naming convention, will have
> a look at updating stuff.
dev and sdev are fine as well, as suggested in the other email.

Re: [PATCH 04/15] ASoC: SDCA: Pass device register map from IRQ alloc to handlers

[Pierre-Louis Bossart]

On 9/8/25 14:56, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 01:49:43PM +0200, Pierre-Louis Bossart wrote:
>> On 9/5/25 16:31, Charles Keepax wrote:
>>> Store a copy of the device register map in the structure for the IRQ
>>> handlers. This will allow the individual IRQ handlers access to the
>>> device level register map if required.
>>
>> Sorry, not following how you plan on sharing access to regmap between
>> parent and child devices.
> 
> They all have a pointer to the device level register map, if some
> function or the device level is using it the regmap lock will be
> held and the others are block.

ok

>>> @@ -445,6 +445,9 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
>>>  
>>>  	info->irq_chip = sdca_irq_chip;
>>>  
>>> +	for (i = 0; i < ARRAY_SIZE(info->irqs); i++)
>>> +		info->irqs[i].device_regmap = regmap;
>>> +
>>
>> Lost on this one as well. A child device could support multiple
>> interrupts, e.g. for buttons and jack, not sure why each interrupt
>> needs a separate pointer to the same identical information.
> 
> Most IRQs don't need custom handling so the core can handle those
> IRQs. In the case of custom IRQs the driver would register some
> private data structure that would contain necessary information
> for the handler such as regmaps. This array of interrupt
> structures is basically that private data. A pointer to this
> structure will be passed to the IRQ when it fires and it needs to
> contain all the stuff the IRQ handler needs, one of those things
> might be the device level register map.

I think I am still looking at interrupts with the standard non-SDCA interrupts in mind. In the existing SoundWire handling, all interrupts are handled one after the other in the same handler. The only case where we could customize the handler is for the 'vendor-defined' interrupt.

But for SDCA we are moving to a different design, aren't we? Each interrupt is handled separately with its own handler, either provided by the core or customized by a vendor, and that handler must be provided with enough context information to access registers. If that description is correct then yes no objection to this copy of the information.

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

On 9/8/25 15:15, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
>> On 9/5/25 16:31, Charles Keepax wrote:
>>> Add helper functions for handling Universal Message Passing (UMP)
>>> buffers on SDCA devices. These are generic mechanisms to pass blocks of
>>> binary data between the host and the device, in both directions. They
>>> are used for things like passing HID descriptors and the File Download
>>> process.
>>
>> Humm, these helpers are very host-centric and don't seem to cover the basic
>> UMP message passing, e.g. for host to device:
>>
>> 1. host puts information in a buffer.
> 
> by calling sdca_ump_write_message

ok
>> 2. host sets ownership to device and waits for ownership to be returned.
> 
> by calling sdca_ump_set_owner_device

sdca_ump_set_owner_device does the ownership change only, there's no wait.

>> 3. device does a bunch of things and returns the ownership to the host.
>> 4. host detects ownership change or times out with a failed transfer error message.
> 
> by receiving an IRQ on the owner control.

but that IRQ may never come, in which case the entire protocol would fail.
>> What am I missing?
> 
> You might need to elaborate a little more on what you think is
> missing. The four functions implement the four operations that
> power a UMP buffer, checking you have ownership, passing
> ownership back to the device, reading data, and writing data.
> 
> One could combine more of the functionality into a single helper,
> but then you start to hit problems where you need to do extra
> bits.
> 
> For example HID, very simple just: check you own the buffer,
> read the message, and pass the buffer back.

I remember one of the early implementations of HID support in a Realtek driver needed some sort of timeout to reset the communication in case the ownership change interrupt never came. It's reasonable to expect that things will go wrong at some point in the communication between host and device, isn't it?

> FDL, has extra steps: check you own the buffer, check the FDL
> status, then write the buffer, then write the FDL response,
> and only then pass the buffer back.

Right, my feedback was only related to the basic UMP support and what happens if the expected ownership isn't signaled. 
I do think we need some sort of software timeout to make sure we don't wait forever on an ownership change. I did see the use of wait_for_completion/complete for the FDL stuff but it should be used at a lower UMP level IMHO.

Re: [PATCH 11/15] ASoC: SDCA: Add FDL library for XU entities

[Pierre-Louis Bossart]

On 9/8/25 15:16, Charles Keepax wrote:
> On Mon, Sep 08, 2025 at 02:14:00PM +0200, Pierre-Louis Bossart wrote:
>>  
>>> +config SND_SOC_SDCA_XU
>>> +	bool "SDCA XU support"
>>> +	depends on SND_SOC_SDCA
>>> +	default y
>>> +	help
>>> +	  This option enables support for the MIPI SDCA XU Entity driver.
>>> +
>>
>> I completely agree on the need for a common library for file
>> download. But that functionality happens to be part of the XU
>> scope. Other things might be added by vendors for secret-sauce
>> controls.
>>
>> IOW, this isn't XU support per se but FDL support only. An
>> actual vendor-specific XU driver would rely on the FDL library
>> and add more things of top.
> 
> We can rename this to be more directly FDL, that seems fine.

ok

Re: [PATCH 04/15] ASoC: SDCA: Pass device register map from IRQ alloc to handlers

[Charles Keepax]

On Tue, Sep 09, 2025 at 03:05:40PM +0200, Pierre-Louis Bossart wrote:
> On 9/8/25 14:56, Charles Keepax wrote:
> > On Mon, Sep 08, 2025 at 01:49:43PM +0200, Pierre-Louis Bossart wrote:
> >> On 9/5/25 16:31, Charles Keepax wrote:
> >> Lost on this one as well. A child device could support multiple
> >> interrupts, e.g. for buttons and jack, not sure why each interrupt
> >> needs a separate pointer to the same identical information.
> > 
> > Most IRQs don't need custom handling so the core can handle those
> > IRQs. In the case of custom IRQs the driver would register some
> > private data structure that would contain necessary information
> > for the handler such as regmaps. This array of interrupt
> > structures is basically that private data. A pointer to this
> > structure will be passed to the IRQ when it fires and it needs to
> > contain all the stuff the IRQ handler needs, one of those things
> > might be the device level register map.
> 
> I think I am still looking at interrupts with the standard
> non-SDCA interrupts in mind. In the existing SoundWire handling,
> all interrupts are handled one after the other in the same
> handler. The only case where we could customize the handler is
> for the 'vendor-defined' interrupt.
> 
> But for SDCA we are moving to a different design, aren't
> we? Each interrupt is handled separately with its own handler,
> either provided by the core or customized by a vendor, and that
> handler must be provided with enough context information to
> access registers. If that description is correct then yes no
> objection to this copy of the information.

Yeah exactly correct, each IRQ gets it own handler and that needs
all the necessary context to handle the IRQ.

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
> On 9/8/25 15:15, Charles Keepax wrote:
> > On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
> >> On 9/5/25 16:31, Charles Keepax wrote:
> >> 4. host detects ownership change or times out with a failed transfer error message.
> > 
> > by receiving an IRQ on the owner control.
> 
> but that IRQ may never come, in which case the entire protocol would fail.
> > 
> > You might need to elaborate a little more on what you think is
> > missing. The four functions implement the four operations that
> > power a UMP buffer, checking you have ownership, passing
> > ownership back to the device, reading data, and writing data.
> > 
> > One could combine more of the functionality into a single helper,
> > but then you start to hit problems where you need to do extra
> > bits.
> > 
> > For example HID, very simple just: check you own the buffer,
> > read the message, and pass the buffer back.
> 
> I remember one of the early implementations of HID support
> in a Realtek driver needed some sort of timeout to reset the
> communication in case the ownership change interrupt never
> came. It's reasonable to expect that things will go wrong at
> some point in the communication between host and device, isn't it?

I will have a look see if I can find that driver and see what is
going on there. But HID seems like a good example (or at least the
event part of HID) of why the wait isn't in the UMP code to me.

The only notification that comes from the device is the the
UMP buffer is being returned to the host, which indicates a HID
event has happened. But there is no time out on HID events, it
could be months between HID events. Like one could set a timeout
and at each timeout just verify the buffer is still set to the
device but that seems very cautious. Or one could check the buffer
is set as expected on boot, but I would really expect the SDCA
reset/boot logic to ensure that is good.

If it is a UMP transfer where the host expects the device to do
something and return that may need a timeout but as that is a
subset of UMP transfers it feels like it makes more sense to
implement the timeouts in the places that need them.

Thanks,
Charles

Re: [PATCH 07/15] ASoC: SDCA: Rely less on the ASoC component in IRQ handling

[Liao, Bard]

On 9/9/2025 4:56 PM, Charles Keepax wrote:
> On Tue, Sep 09, 2025 at 09:42:20AM +0800, Liao, Bard wrote:
>> On 9/8/2025 10:43 PM, Charles Keepax wrote:
>>> On Mon, Sep 08, 2025 at 01:56:57PM +0100, Charles Keepax wrote:
>>>> On Mon, Sep 08, 2025 at 06:27:40PM +0800, Liao, Bard wrote:
>>>>> On 9/5/2025 10:31 PM, Charles Keepax wrote:
>>>>>> -int sdca_irq_data_populate(struct snd_soc_component *component,
>>>>>> +int sdca_irq_data_populate(struct device *dev, struct regmap *regmap,
>>>>>> +			   struct snd_soc_component *component,
>>>>>>  			   struct sdca_function_data *function,
>>>>>>  			   struct sdca_entity *entity,
>>>>>>  			   struct sdca_control *control,
>>>>>>  			   struct sdca_interrupt *interrupt)
>>>>>>  {
>>>>>> -	struct device *dev = component->dev;
>>>>>
>>>>> Previously, we assume 'component' will never be null.
>>>>>
>>>>>>  	const char *name;
>>>>>>  
>>>>>> +	if (!dev && component)
>>>>>
>>>>> But now we test 'component'. If we don't assume 'component' is not null
>>>>> any more, we could test 'component' at the very beginning of this function.
>>>>
>>>> Good spot, that was missed when moving from an older version of
>>>> the code. I will get that fixed up.
>>>
>>> Apologies I had slightly misinterpretted this, and thought you
>>> meant we were dereferencing component before testing it in
>>> sdca_irq_data_populate, but we arn't.
>>
>> No, my question is about whether component can be NULL.
> 
> The expectation would be component can not be NULL in
> sdca_irq_populate(), but it could be NULL inside
> sdca_irq_data_populate(), if called from somewhere else.
> 
>> Understood. I had the question because the change below.
>>
>> +	interrupt->dev = dev;
>> +	if (!regmap && component)
>> +		interrupt->function_regmap = component->regmap;
>> +	else
>> +		interrupt->function_regmap = regmap;
>>
>> If both regmap and component are null, the interrupt->function_regmap
>> will be NULL. Is it valid?
> 
> Ok... yeah I see. I guess it depends on the IRQ handler that
> ultimately gets registered. In most cases I would expect the
> handler to need a regmap, however, we don't really validate
> any of the data that is purely for the handler. For example we
> don't validate function/entity/control either, we check dev for
> NULL because it is used for devm allocation of the name string,
> not because it is data for the handler. I think doing it this
> way saves us making assumptions, once we start doing so we start
> to constraint what can be done through the API.

Thanks for explanation. I got it now.


> 
> Thanks,
> Charles
> 

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

On 9/9/25 18:52, Charles Keepax wrote:
> On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
>> On 9/8/25 15:15, Charles Keepax wrote:
>>> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
>>>> On 9/5/25 16:31, Charles Keepax wrote:
>>>> 4. host detects ownership change or times out with a failed transfer error message.
>>>
>>> by receiving an IRQ on the owner control.
>>
>> but that IRQ may never come, in which case the entire protocol would fail.
>>>
>>> You might need to elaborate a little more on what you think is
>>> missing. The four functions implement the four operations that
>>> power a UMP buffer, checking you have ownership, passing
>>> ownership back to the device, reading data, and writing data.
>>>
>>> One could combine more of the functionality into a single helper,
>>> but then you start to hit problems where you need to do extra
>>> bits.
>>>
>>> For example HID, very simple just: check you own the buffer,
>>> read the message, and pass the buffer back.
>>
>> I remember one of the early implementations of HID support
>> in a Realtek driver needed some sort of timeout to reset the
>> communication in case the ownership change interrupt never
>> came. It's reasonable to expect that things will go wrong at
>> some point in the communication between host and device, isn't it?
> 
> I will have a look see if I can find that driver and see what is
> going on there. But HID seems like a good example (or at least the
> event part of HID) of why the wait isn't in the UMP code to me.
> 
> The only notification that comes from the device is the the
> UMP buffer is being returned to the host, which indicates a HID
> event has happened. But there is no time out on HID events, it
> could be months between HID events. Like one could set a timeout
> and at each timeout just verify the buffer is still set to the
> device but that seems very cautious. Or one could check the buffer
> is set as expected on boot, but I would really expect the SDCA
> reset/boot logic to ensure that is good.
> 
> If it is a UMP transfer where the host expects the device to do
> something and return that may need a timeout but as that is a
> subset of UMP transfers it feels like it makes more sense to
> implement the timeouts in the places that need them.

You have a point that in the case where the host waits for an event, then the notion of timeout is irrelevant.

What I was referring to was the case where the host writes a message and waits for a notification that the message has been handled. 
This could be a write to a buffer or a read from a buffer. That's standard IPC stuff, and all existing cases of such message-passing IPC do have a timeout built-in. The communication is serial by nature since there is a single signaling mechanism, so the host has to wait before sending another message.

I am not sure it's a great idea to leave the timeout handling at a higher level, because you will have to deal with other sources of information. Once a message has been sent with UMP, there will be additional signaling that the action requested by the host was completed on the peripheral side. In the case of FDL, once the firmware buffers have been transferred, possibly in different chunks, then the peripheral might have to perform an internal reboot/reset. That takes time and it's a different level of timeout.

My take is that the UMP should implement basic timeout capabilities for just 'buffer passed/ownership changed' transition. Things will go wrong and you want information on where they went wrong. Implementing timeouts at a higher level makes it harder to debug/root cause.

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Wed, Sep 10, 2025 at 02:09:14PM +0200, Pierre-Louis Bossart wrote:
> On 9/9/25 18:52, Charles Keepax wrote:
> > On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
> >> On 9/8/25 15:15, Charles Keepax wrote:
> >>> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
> >>>> On 9/5/25 16:31, Charles Keepax wrote:
> > The only notification that comes from the device is the the
> > UMP buffer is being returned to the host, which indicates a HID
> > event has happened. But there is no time out on HID events, it
> > could be months between HID events. Like one could set a timeout
> > and at each timeout just verify the buffer is still set to the
> > device but that seems very cautious. Or one could check the buffer
> > is set as expected on boot, but I would really expect the SDCA
> > reset/boot logic to ensure that is good.
> > 
> > If it is a UMP transfer where the host expects the device to do
> > something and return that may need a timeout but as that is a
> > subset of UMP transfers it feels like it makes more sense to
> > implement the timeouts in the places that need them.
> 
> You have a point that in the case where the host waits for an
> event, then the notion of timeout is irrelevant.
>
> What I was referring to was the case where the host writes
> a message and waits for a notification that the message has
> been handled.
> This could be a write to a buffer or a read from a buffer.
> That's standard IPC stuff, and all existing cases of such
> message-passing IPC do have a timeout built-in. The communication
> is serial by nature since there is a single signaling mechanism,
> so the host has to wait before sending another message.
>
> I am not sure it's a great idea to leave the timeout handling
> at a higher level, because you will have to deal with other
> sources of information. Once a message has been sent with UMP,
> there will be additional signaling that the action requested by
> the host was completed on the peripheral side. In the case of
> FDL, once the firmware buffers have been transferred, possibly
> in different chunks, then the peripheral might have to perform
> an internal reboot/reset. That takes time and it's a different
> level of timeout.
>
> My take is that the UMP should implement basic timeout
> capabilities for just 'buffer passed/ownership changed'
> transition. Things will go wrong and you want information on
> where they went wrong. Implementing timeouts at a higher level
> makes it harder to debug/root cause.

I guess my slight concern here is that SDCA is a huge fan
of corner cases and everything being slightly inconsistent
with itself. So for example we add two things in this patch
chain HID and FDL. HID very clearly wants no timeouts, there
is no waiting for responses, its just get event, do stuff.
And for FDL the vast majority of the signalling is outside of
the actual UMP buffer through the FDL state control. I am sure
once we get around to Algorithm Extension, History Buffers and
Device-to-Device messaging they will have their own set of odd
requirements. So I liked the APIs being fairly low level and
implementing the basic operations, as it leaves space for
implementing the weirdness.

I guess perhaps the first question here, is this something we can
add incrementally or are you seeing this as a bit more fundamental
and needs done before we progress this chain?  (we are getting
a little nervous that devices are going to be shipping in the
not too distant future that are going to require this stuff).

Second question would probably be could we get a little more of a
concrete example of what you are looking for. Options I can see
would be to add the options of using a blocking type API, maybe
something like:

sdca_ump_notify_owner() - this could probably be part of
                          sdca_ump_get_owner_host()
sdca_ump_wait_for_owner()
sdca_ump_send_message_and_wait() - this would like be a wrapper
                                   for a write and the first two.

But I am a little hesitant to switch the FDL process over this,
has to start firing off work queues etc. and everything gets a
little more complex.

Or one could add something like:

sdca_ump_cancel_timeout()
sdca_ump_schedule_timeout(callback, timeout)

This probably integrates nicer with the IRQ driven way the FDL
is implemented at the moment.

Would something like one of those be what you are looking for? Or
are you really looking for a much higher level API with some sort
of message queue? Although if going in that way I get very very
nervous about all the SDCA weirdness.

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

On 9/10/25 15:37, Charles Keepax wrote:
> On Wed, Sep 10, 2025 at 02:09:14PM +0200, Pierre-Louis Bossart wrote:
>> On 9/9/25 18:52, Charles Keepax wrote:
>>> On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
>>>> On 9/8/25 15:15, Charles Keepax wrote:
>>>>> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
>>>>>> On 9/5/25 16:31, Charles Keepax wrote:
>>> The only notification that comes from the device is the the
>>> UMP buffer is being returned to the host, which indicates a HID
>>> event has happened. But there is no time out on HID events, it
>>> could be months between HID events. Like one could set a timeout
>>> and at each timeout just verify the buffer is still set to the
>>> device but that seems very cautious. Or one could check the buffer
>>> is set as expected on boot, but I would really expect the SDCA
>>> reset/boot logic to ensure that is good.
>>>
>>> If it is a UMP transfer where the host expects the device to do
>>> something and return that may need a timeout but as that is a
>>> subset of UMP transfers it feels like it makes more sense to
>>> implement the timeouts in the places that need them.
>>
>> You have a point that in the case where the host waits for an
>> event, then the notion of timeout is irrelevant.
>>
>> What I was referring to was the case where the host writes
>> a message and waits for a notification that the message has
>> been handled.
>> This could be a write to a buffer or a read from a buffer.
>> That's standard IPC stuff, and all existing cases of such
>> message-passing IPC do have a timeout built-in. The communication
>> is serial by nature since there is a single signaling mechanism,
>> so the host has to wait before sending another message.
>>
>> I am not sure it's a great idea to leave the timeout handling
>> at a higher level, because you will have to deal with other
>> sources of information. Once a message has been sent with UMP,
>> there will be additional signaling that the action requested by
>> the host was completed on the peripheral side. In the case of
>> FDL, once the firmware buffers have been transferred, possibly
>> in different chunks, then the peripheral might have to perform
>> an internal reboot/reset. That takes time and it's a different
>> level of timeout.
>>
>> My take is that the UMP should implement basic timeout
>> capabilities for just 'buffer passed/ownership changed'
>> transition. Things will go wrong and you want information on
>> where they went wrong. Implementing timeouts at a higher level
>> makes it harder to debug/root cause.
> 
> I guess my slight concern here is that SDCA is a huge fan
> of corner cases and everything being slightly inconsistent
> with itself. So for example we add two things in this patch
> chain HID and FDL. HID very clearly wants no timeouts, there
> is no waiting for responses, its just get event, do stuff.
> And for FDL the vast majority of the signalling is outside of
> the actual UMP buffer through the FDL state control. I am sure
> once we get around to Algorithm Extension, History Buffers and
> Device-to-Device messaging they will have their own set of odd
> requirements. So I liked the APIs being fairly low level and
> implementing the basic operations, as it leaves space for
> implementing the weirdness.
> 
> I guess perhaps the first question here, is this something we can
> add incrementally or are you seeing this as a bit more fundamental
> and needs done before we progress this chain?  (we are getting
> a little nervous that devices are going to be shipping in the
> not too distant future that are going to require this stuff).
> 
> Second question would probably be could we get a little more of a
> concrete example of what you are looking for. Options I can see
> would be to add the options of using a blocking type API, maybe
> something like:
> 
> sdca_ump_notify_owner() - this could probably be part of
>                           sdca_ump_get_owner_host()
> sdca_ump_wait_for_owner()
> sdca_ump_send_message_and_wait() - this would like be a wrapper
>                                    for a write and the first two.
> 
> But I am a little hesitant to switch the FDL process over this,
> has to start firing off work queues etc. and everything gets a
> little more complex.
> 
> Or one could add something like:
> 
> sdca_ump_cancel_timeout()
> sdca_ump_schedule_timeout(callback, timeout)
> 
> This probably integrates nicer with the IRQ driven way the FDL
> is implemented at the moment.
> 
> Would something like one of those be what you are looking for? Or
> are you really looking for a much higher level API with some sort
> of message queue? Although if going in that way I get very very
> nervous about all the SDCA weirdness.

Oh I wasn't thinking at all of a message queue with a higher-level API, more something along the lines of the SOF/cAVS IPC based on doorbells.

You probably want an async mechanism where the set_owner() returns immediately and is followed by a wait with a configurable timeout. I don't really get the nuance between your two proposals but they go in the direction I had in mind.

That said, I do think you'll have to deal with workqueues, not sure how else to deal with the protocol. If you have an interrupt that requires a follow-up UMP-based check, things could get messy with an interrupt blocking another one. We had similar problems with the SOF IPC and for the standard SoundWire interrupts. Best to keep interrupt handlers simple and signal stuff to do for workqueues.

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Wed, Sep 10, 2025 at 04:29:40PM +0200, Pierre-Louis Bossart wrote:
> On 9/10/25 15:37, Charles Keepax wrote:
> > On Wed, Sep 10, 2025 at 02:09:14PM +0200, Pierre-Louis Bossart wrote:
> >> On 9/9/25 18:52, Charles Keepax wrote:
> >>> On Tue, Sep 09, 2025 at 03:14:00PM +0200, Pierre-Louis Bossart wrote:
> >>>> On 9/8/25 15:15, Charles Keepax wrote:
> >>>>> On Mon, Sep 08, 2025 at 02:02:09PM +0200, Pierre-Louis Bossart wrote:
> >>>>>> On 9/5/25 16:31, Charles Keepax wrote:
> > Second question would probably be could we get a little more of a
> > concrete example of what you are looking for. Options I can see
> > would be to add the options of using a blocking type API, maybe
> > something like:
> > 
> > sdca_ump_notify_owner() - this could probably be part of
> >                           sdca_ump_get_owner_host()
> > sdca_ump_wait_for_owner()
> > sdca_ump_send_message_and_wait() - this would like be a wrapper
> >                                    for a write and the first two.
> > 
> > But I am a little hesitant to switch the FDL process over this,
> > has to start firing off work queues etc. and everything gets a
> > little more complex.
> > 
> > Or one could add something like:
> > 
> > sdca_ump_cancel_timeout()
> > sdca_ump_schedule_timeout(callback, timeout)
> > 
> > This probably integrates nicer with the IRQ driven way the FDL
> > is implemented at the moment.
> > 
> > Would something like one of those be what you are looking for? Or
> > are you really looking for a much higher level API with some sort
> > of message queue? Although if going in that way I get very very
> > nervous about all the SDCA weirdness.
> 
> Oh I wasn't thinking at all of a message queue with a
> higher-level API, more something along the lines of the SOF/cAVS
> IPC based on doorbells.

I am not super familiar with the inner workings of SOF but at
first glance it does look like exactly the thing I was hoping to
avoid :-)

> You probably want an async mechanism where the set_owner()
> returns immediately and is followed by a wait with a configurable
> timeout. I don't really get the nuance between your two proposals
> but they go in the direction I had in mind.

The difference was intended to be the first is a blocking API
(ie. function returns on either owner being transferred back to
you or the timeout expires). The second was intended to be async
and based on a callback ie. returns immediately and the callback
is called when the timeout expires so you can run whatever error
handling you want.

> That said, I do think you'll have to deal with workqueues,
> not sure how else to deal with the protocol. If you have an
> interrupt that requires a follow-up UMP-based check, things
> could get messy with an interrupt blocking another one. We had
> similar problems with the SOF IPC and for the standard SoundWire
> interrupts. Best to keep interrupt handlers simple and signal
> stuff to do for workqueues.

Definitely agree that blocking in the IRQs is not what we want,
hence my previous comments about preferring the callback based
option.

For the most part HID and FDL are extremely device led
processes. We don't really need to keep super complex state on the
host side as we don't initiate anything, we are just responding
to what the device wants to do. To my mind keeping complex state
on the host side likely increases the chance something goes wrong.

I will have a think see if I can come up with a nice way to
integrate a timeout to individual UMP owner transfers, but I am
not exactly sure how much value this adds for the currently
implemented features. HID won't use this, and FDL already has a
timeout for the FDL process, so one still sees if it failed and
as you can see the status and response pairs debugging should be
fairly easy.

Once these timeouts exist we have to pick values for them,
because SDCA is a huge huge fan of asking you to wait for
something but not saying for how long. If you have any thoughts
on this that would be appreciated too, but mostly I will just
pluck some vaguely reasonable sounding values as I did for the
existing FDL timeouts.

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

>>> Second question would probably be could we get a little more of a
>>> concrete example of what you are looking for. Options I can see
>>> would be to add the options of using a blocking type API, maybe
>>> something like:
>>>
>>> sdca_ump_notify_owner() - this could probably be part of
>>>                           sdca_ump_get_owner_host()
>>> sdca_ump_wait_for_owner()
>>> sdca_ump_send_message_and_wait() - this would like be a wrapper
>>>                                    for a write and the first two.
>>>
>>> But I am a little hesitant to switch the FDL process over this,
>>> has to start firing off work queues etc. and everything gets a
>>> little more complex.
>>>
>>> Or one could add something like:
>>>
>>> sdca_ump_cancel_timeout()
>>> sdca_ump_schedule_timeout(callback, timeout)
>>>
>>> This probably integrates nicer with the IRQ driven way the FDL
>>> is implemented at the moment.
>>>
>>> Would something like one of those be what you are looking for? Or
>>> are you really looking for a much higher level API with some sort
>>> of message queue? Although if going in that way I get very very
>>> nervous about all the SDCA weirdness.
>>
>> Oh I wasn't thinking at all of a message queue with a
>> higher-level API, more something along the lines of the SOF/cAVS
>> IPC based on doorbells.
> 
> I am not super familiar with the inner workings of SOF but at
> first glance it does look like exactly the thing I was hoping to
> avoid :-)
> 
>> You probably want an async mechanism where the set_owner()
>> returns immediately and is followed by a wait with a configurable
>> timeout. I don't really get the nuance between your two proposals
>> but they go in the direction I had in mind.
> 
> The difference was intended to be the first is a blocking API
> (ie. function returns on either owner being transferred back to
> you or the timeout expires). The second was intended to be async
> and based on a callback ie. returns immediately and the callback
> is called when the timeout expires so you can run whatever error
> handling you want.
> 
>> That said, I do think you'll have to deal with workqueues,
>> not sure how else to deal with the protocol. If you have an
>> interrupt that requires a follow-up UMP-based check, things
>> could get messy with an interrupt blocking another one. We had
>> similar problems with the SOF IPC and for the standard SoundWire
>> interrupts. Best to keep interrupt handlers simple and signal
>> stuff to do for workqueues.
> 
> Definitely agree that blocking in the IRQs is not what we want,
> hence my previous comments about preferring the callback based
> option.

ok

> For the most part HID and FDL are extremely device led
> processes. We don't really need to keep super complex state on the
> host side as we don't initiate anything, we are just responding
> to what the device wants to do. To my mind keeping complex state
> on the host side likely increases the chance something goes wrong.
> 
> I will have a think see if I can come up with a nice way to
> integrate a timeout to individual UMP owner transfers, but I am
> not exactly sure how much value this adds for the currently
> implemented features. HID won't use this, and FDL already has a
> timeout for the FDL process, so one still sees if it failed and
> as you can see the status and response pairs debugging should be
> fairly easy.
> 
> Once these timeouts exist we have to pick values for them,
> because SDCA is a huge huge fan of asking you to wait for
> something but not saying for how long. If you have any thoughts
> on this that would be appreciated too, but mostly I will just
> pluck some vaguely reasonable sounding values as I did for the
> existing FDL timeouts.

The SDCA 1.0 spec defines DisCo properties for UMP timeouts, just do a search for "ownership-transition-max-delay". Table 139 page 236 defines this property specifically for XUs and hence the low-level protocol FDL is based on.

The description reads:

"
The maximum time allowed for Device to change the
ownership from Device to Host in an Rx UMP when Host
Software is waiting for the owner to change back to Host.
"

As usual it's quite possible that platform firmware is broken with bad values, but the design intent was to provide a timeout value for software to use. Ignoring these properties doesn't seem quite right to me...

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Thu, Sep 11, 2025 at 02:33:46PM +0200, Pierre-Louis Bossart wrote:
> > Once these timeouts exist we have to pick values for them,
> > because SDCA is a huge huge fan of asking you to wait for
> > something but not saying for how long. If you have any thoughts
> > on this that would be appreciated too, but mostly I will just
> > pluck some vaguely reasonable sounding values as I did for the
> > existing FDL timeouts.
> 
> The SDCA 1.0 spec defines DisCo properties for UMP timeouts,
> just do a search for "ownership-transition-max-delay". Table 139
> page 236 defines this property specifically for XUs and hence
> the low-level protocol FDL is based on.
> 
> The description reads:
> 
> "
> The maximum time allowed for Device to change the
> ownership from Device to Host in an Rx UMP when Host
> Software is waiting for the owner to change back to Host.
> "
> 
> As usual it's quite possible that platform firmware is broken
> with bad values, but the design intent was to provide a timeout
> value for software to use. Ignoring these properties doesn't
> seem quite right to me...

No, I had missed this property! That is excellent. Don't suppose
you have a similar magic touch with the "Wait until there are
no new requests for File Download" in the Class Software Load
Sequence?

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

On 9/11/25 15:07, Charles Keepax wrote:
> On Thu, Sep 11, 2025 at 02:33:46PM +0200, Pierre-Louis Bossart wrote:
>>> Once these timeouts exist we have to pick values for them,
>>> because SDCA is a huge huge fan of asking you to wait for
>>> something but not saying for how long. If you have any thoughts
>>> on this that would be appreciated too, but mostly I will just
>>> pluck some vaguely reasonable sounding values as I did for the
>>> existing FDL timeouts.
>>
>> The SDCA 1.0 spec defines DisCo properties for UMP timeouts,
>> just do a search for "ownership-transition-max-delay". Table 139
>> page 236 defines this property specifically for XUs and hence
>> the low-level protocol FDL is based on.
>>
>> The description reads:
>>
>> "
>> The maximum time allowed for Device to change the
>> ownership from Device to Host in an Rx UMP when Host
>> Software is waiting for the owner to change back to Host.
>> "
>>
>> As usual it's quite possible that platform firmware is broken
>> with bad values, but the design intent was to provide a timeout
>> value for software to use. Ignoring these properties doesn't
>> seem quite right to me...
> 
> No, I had missed this property! That is excellent. Don't suppose
> you have a similar magic touch with the "Wait until there are
> no new requests for File Download" in the Class Software Load
> Sequence?

Glad I was able to help!

The FDL state machine is rather complicated, I can't pretend understanding it fully :-(

My limited understanding is that the Host doesn't control which firmware files need to be downloaded. The Device will provide detailed information back to the host. That's how I interpret the vague statement in Figure 213:

"Device interprets
FDL_Host_request and perhaps
requests one or more new
FDL_Set_Index(es)."

IOW the number of sets to download is indicated in the FDLD_Needs_set control. Based on the bit pattern in Table 240, there can be up to 8 sets (3 bits).

The main issue I have is that the files will be transmitted by chunks. I am not quite sure who defines the size of the chunks, but each chunk should be handled by the same "ownership-transition-max-delay" property. It's rather odd that the timeout doesn't depend on the size of the chunk, but assuming this is correct the time to download should be bound by number of sets x number of chunks per set x ownership-transition-max-delay.

That's the limited extended of my FDL "magic touch" I am afraid...

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Charles Keepax]

On Fri, Sep 12, 2025 at 12:15:45PM +0200, Pierre-Louis Bossart wrote:
> On 9/11/25 15:07, Charles Keepax wrote:
> > On Thu, Sep 11, 2025 at 02:33:46PM +0200, Pierre-Louis Bossart wrote:
> >>> Once these timeouts exist we have to pick values for them,
> >>> because SDCA is a huge huge fan of asking you to wait for
> >>> something but not saying for how long. If you have any thoughts
> >>> on this that would be appreciated too, but mostly I will just
> >>> pluck some vaguely reasonable sounding values as I did for the
> >>> existing FDL timeouts.
> >>
> >> The SDCA 1.0 spec defines DisCo properties for UMP timeouts,
> >> just do a search for "ownership-transition-max-delay". Table 139
> >> page 236 defines this property specifically for XUs and hence
> >> the low-level protocol FDL is based on.
> >>
> >> The description reads:
> >>
> >> "
> >> The maximum time allowed for Device to change the
> >> ownership from Device to Host in an Rx UMP when Host
> >> Software is waiting for the owner to change back to Host.
> >> "
> >>
> >> As usual it's quite possible that platform firmware is broken
> >> with bad values, but the design intent was to provide a timeout
> >> value for software to use. Ignoring these properties doesn't
> >> seem quite right to me...
> > 
> > No, I had missed this property! That is excellent. Don't suppose
> > you have a similar magic touch with the "Wait until there are
> > no new requests for File Download" in the Class Software Load
> > Sequence?
> 
> Glad I was able to help!
> 
> The FDL state machine is rather complicated, I can't pretend
> understanding it fully :-(

Ha, you and me both.

> My limited understanding is that the Host doesn't control which
> firmware files need to be downloaded. The Device will provide
> detailed information back to the host. That's how I interpret
> the vague statement in Figure 213:
> 
> "Device interprets
> FDL_Host_request and perhaps
> requests one or more new
> FDL_Set_Index(es)."
> 
> IOW the number of sets to download is indicated in the
> FDLD_Needs_set control. Based on the bit pattern in Table 240,
> there can be up to 8 sets (3 bits).
> 
> The main issue I have is that the files will be transmitted
> by chunks. I am not quite sure who defines the size of
> the chunks, but each chunk should be handled by the same
> "ownership-transition-max-delay" property. It's rather odd that
> the timeout doesn't depend on the size of the chunk, but assuming
> this is correct the time to download should be bound by number of
> sets x number of chunks per set x ownership-transition-max-delay.

Our devices only download a since chunk so we haven't added
support yet for the indirect FDL yet. Which keeps things a
little simpler and means we can file work out the chunk size as a
problem for later. Although my rough interpretation of that bit
is that the chunk size is defined by the UMP buffer itself, so
basically the host fills the whole buffer each time, but if the
file is larger than the buffer it takes a few goes to send it all.

> That's the limited extended of my FDL "magic touch" I am afraid...

Yeah mainly my question was that it says "wait until" but I have
never been able to define what that wait is. One possible
interpretation is as soon as you see the device not requesting
FDL you can progress, but perhaps a safer interpretation is if
you see X milliseconds without an FDL request you can progress
which is what we have gone with in the code for now. Was really
just hoping you were going to be like "you also missed this other
useful DisCo property" :-)

But I am just about to push up a v2 so we can carry on the chats
on those.

Thanks,
Charles

Re: [PATCH 09/15] ASoC: SDCA: Add UMP buffer helper functions

[Pierre-Louis Bossart]

> Yeah mainly my question was that it says "wait until" but I have
> never been able to define what that wait is. One possible
> interpretation is as soon as you see the device not requesting
> FDL you can progress, but perhaps a safer interpretation is if
> you see X milliseconds without an FDL request you can progress
> which is what we have gone with in the code for now. Was really
> just hoping you were going to be like "you also missed this other
> useful DisCo property" :-)

I don't think DisCo defined anything related to FDL. 

The only thing I see in the spec are Device-initiated status changes, i.e. the loops would look like

do {
	do {
		send_chunk()
		wait for FDLD_Chunk_OK();
	} while (!FDLD_File_OK)
	get_next file();
} while (!FDLD_Complete or !FDLD_Request_Reset) {

BTW, FDLD_Request_Reset (bxxx1Rxxx) and FDLD_Need_Reset (b0001r000) are probably the same concept using different names, not sure if this was clarified in a edit.

Like you said this can probably be discussed in a follow-up thread, it'll take time to limit and adjust the many degrees of freedom in the spec to what vendors actually implemented :-)