[PATCH v5 0/6] Add DAPM/ASoC helpers to create SDCA drivers

[PATCH v5 0/6] Add DAPM/ASoC helpers to create SDCA drivers

[Charles Keepax]

Add helper functions to add DAPM widgets, routes, ALSA controls,
and DAI drivers, these will be used to create SDCA function device
drivers.

This series should provide most of the core functionality needed to
get a device registered and have a working DAPM graph within the
device. There are some features that still need additional work, these
are marked with FIXMEs in the code. The two main things are SDCA
Clock Muxes (not used in our devices and needs some ASoC core work),
and better support for more complex SDCA volume control definitions
(our parts have fairly simple volumes, and SDCA has a large amount of
flexibility in how the volume control is specified).

The next steps in the process are to add helpers for the DAI ops
themselves, some IRQ handling, and firmware download. And finally we
should be able to actually add the SDCA class driver itself.

Thanks,
Charles

Changes since v4:
 - Add include of soc-dai.h, this should be included directly.
 - Remove unnecessary MODULE_LICENSE() and MODULE_DESCRIPTION() macros.
 - Move the error message into selector_find_control to better match
   the other search functions.

Charles Keepax (6):
  ASoC: SDCA: Remove regmap module macros
  ASoC: SDCA: Move allocation of PDE delays array
  ASoC: dapm: Add component level pin switches
  ASoC: SDCA: Create DAPM widgets and routes from DisCo
  ASoC: SDCA: Create ALSA controls from DisCo
  ASoC: SDCA: Create DAI drivers from DisCo

 include/sound/sdca_asoc.h       |   42 +
 include/sound/sdca_function.h   |   69 ++
 include/sound/soc-dapm.h        |    4 +
 sound/soc/sdca/Makefile         |    2 +-
 sound/soc/sdca/sdca_asoc.c      | 1291 +++++++++++++++++++++++++++++++
 sound/soc/sdca/sdca_functions.c |   10 +-
 sound/soc/sdca/sdca_regmap.c    |    3 -
 sound/soc/soc-dapm.c            |   84 +-
 8 files changed, 1481 insertions(+), 24 deletions(-)
 create mode 100644 include/sound/sdca_asoc.h
 create mode 100644 sound/soc/sdca/sdca_asoc.c

-- 
2.39.5

[PATCH v5 1/6] ASoC: SDCA: Remove regmap module macros

[Charles Keepax]

There is no need to include MODULE_LICENSE() and MODULE_DESCRIPTION() in
sdca_regmap.c as this file is part of a larger module that already
defines these.

Fixes: e3f7caf74b79 ("ASoC: SDCA: Add generic regmap SDCA helpers")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

New since v4.

 sound/soc/sdca/sdca_regmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sound/soc/sdca/sdca_regmap.c b/sound/soc/sdca/sdca_regmap.c
index 4b78188cfcebd..66e7eee7d7f49 100644
--- a/sound/soc/sdca/sdca_regmap.c
+++ b/sound/soc/sdca/sdca_regmap.c
@@ -316,6 +316,3 @@ int sdca_regmap_write_defaults(struct device *dev, struct regmap *regmap,
 	return 0;
 }
 EXPORT_SYMBOL_NS(sdca_regmap_write_defaults, "SND_SOC_SDCA");
-
-MODULE_LICENSE("GPL");
-MODULE_DESCRIPTION("SDCA library");
-- 
2.39.5

[PATCH v5 2/6] ASoC: SDCA: Move allocation of PDE delays array

[Charles Keepax]

Move the allocation of the PDE delays array until after the size has
been adjusted, this saves an additional division and simplifies the
code slightly.

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

No changes since v4.

 sound/soc/sdca/sdca_functions.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 493f390f087ad..64ac264438907 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -1105,12 +1105,6 @@ static int find_sdca_entity_pde(struct device *dev,
 		return -EINVAL;
 	}
 
-	/* There are 3 values per delay */
-	delays = devm_kcalloc(dev, num_delays / mult_delay,
-			      sizeof(*delays), GFP_KERNEL);
-	if (!delays)
-		return -ENOMEM;
-
 	delay_list = kcalloc(num_delays, sizeof(*delay_list), GFP_KERNEL);
 	if (!delay_list)
 		return -ENOMEM;
@@ -1121,6 +1115,10 @@ static int find_sdca_entity_pde(struct device *dev,
 
 	num_delays /= mult_delay;
 
+	delays = devm_kcalloc(dev, num_delays, sizeof(*delays), GFP_KERNEL);
+	if (!delays)
+		return -ENOMEM;
+
 	for (i = 0, j = 0; i < num_delays; i++) {
 		delays[i].from_ps = delay_list[j++];
 		delays[i].to_ps = delay_list[j++];
-- 
2.39.5

[PATCH v5 3/6] ASoC: dapm: Add component level pin switches

[Charles Keepax]

The core currently supports pin switches for source/sink widgets, but
only at the card level. SDCA components specify the fabric at the
level of the individual components, to support this add helpers to
allow component level pin switches.

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

No changes since v4.

 include/sound/soc-dapm.h |  4 ++
 sound/soc/soc-dapm.c     | 84 +++++++++++++++++++++++++++++++++-------
 2 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index af802ef536e73..400584474bc8b 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -445,6 +445,10 @@ int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *uncontrol);
 int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *uncontrol);
+int snd_soc_dapm_get_component_pin_switch(struct snd_kcontrol *kcontrol,
+					  struct snd_ctl_elem_value *uncontrol);
+int snd_soc_dapm_put_component_pin_switch(struct snd_kcontrol *kcontrol,
+					  struct snd_ctl_elem_value *uncontrol);
 int snd_soc_dapm_new_controls(struct snd_soc_dapm_context *dapm,
 	const struct snd_soc_dapm_widget *widget, unsigned int num);
 struct snd_soc_dapm_widget *snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index b7818388984e3..f26f9e9d7ce74 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3626,11 +3626,25 @@ int snd_soc_dapm_info_pin_switch(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_info_pin_switch);
 
+static int __snd_soc_dapm_get_pin_switch(struct snd_soc_dapm_context *dapm,
+					 const char *pin,
+					 struct snd_ctl_elem_value *ucontrol)
+{
+	snd_soc_dapm_mutex_lock(dapm);
+	ucontrol->value.integer.value[0] = snd_soc_dapm_get_pin_status(dapm, pin);
+	snd_soc_dapm_mutex_unlock(dapm);
+
+	return 0;
+}
+
 /**
  * snd_soc_dapm_get_pin_switch - Get information for a pin switch
  *
  * @kcontrol: mixer control
  * @ucontrol: Value
+ *
+ * Callback to provide information for a pin switch added at the card
+ * level.
  */
 int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol)
@@ -3638,40 +3652,82 @@ int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
 	const char *pin = (const char *)kcontrol->private_value;
 
-	snd_soc_dapm_mutex_lock(card);
+	return __snd_soc_dapm_get_pin_switch(&card->dapm, pin, ucontrol);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_get_pin_switch);
 
-	ucontrol->value.integer.value[0] =
-		snd_soc_dapm_get_pin_status(&card->dapm, pin);
+/**
+ * snd_soc_dapm_get_component_pin_switch - Get information for a pin switch
+ *
+ * @kcontrol: mixer control
+ * @ucontrol: Value
+ *
+ * Callback to provide information for a pin switch added at the component
+ * level.
+ */
+int snd_soc_dapm_get_component_pin_switch(struct snd_kcontrol *kcontrol,
+					  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	const char *pin = (const char *)kcontrol->private_value;
 
-	snd_soc_dapm_mutex_unlock(card);
+	return __snd_soc_dapm_get_pin_switch(&component->dapm, pin, ucontrol);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_get_component_pin_switch);
 
-	return 0;
+static int __snd_soc_dapm_put_pin_switch(struct snd_soc_dapm_context *dapm,
+					 const char *pin,
+					 struct snd_ctl_elem_value *ucontrol)
+{
+	int ret;
+
+	snd_soc_dapm_mutex_lock(dapm);
+	ret = __snd_soc_dapm_set_pin(dapm, pin, !!ucontrol->value.integer.value[0]);
+	snd_soc_dapm_mutex_unlock(dapm);
+
+	snd_soc_dapm_sync(dapm);
+
+	return ret;
 }
-EXPORT_SYMBOL_GPL(snd_soc_dapm_get_pin_switch);
 
 /**
  * snd_soc_dapm_put_pin_switch - Set information for a pin switch
  *
  * @kcontrol: mixer control
  * @ucontrol: Value
+ *
+ * Callback to provide information for a pin switch added at the card
+ * level.
  */
 int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_value *ucontrol)
 {
 	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
 	const char *pin = (const char *)kcontrol->private_value;
-	int ret;
-
-	snd_soc_dapm_mutex_lock(card);
-	ret = __snd_soc_dapm_set_pin(&card->dapm, pin,
-				     !!ucontrol->value.integer.value[0]);
-	snd_soc_dapm_mutex_unlock(card);
 
-	snd_soc_dapm_sync(&card->dapm);
-	return ret;
+	return __snd_soc_dapm_put_pin_switch(&card->dapm, pin, ucontrol);
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_pin_switch);
 
+/**
+ * snd_soc_dapm_put_component_pin_switch - Set information for a pin switch
+ *
+ * @kcontrol: mixer control
+ * @ucontrol: Value
+ *
+ * Callback to provide information for a pin switch added at the component
+ * level.
+ */
+int snd_soc_dapm_put_component_pin_switch(struct snd_kcontrol *kcontrol,
+					  struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	const char *pin = (const char *)kcontrol->private_value;
+
+	return __snd_soc_dapm_put_pin_switch(&component->dapm, pin, ucontrol);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_put_component_pin_switch);
+
 struct snd_soc_dapm_widget *
 snd_soc_dapm_new_control_unlocked(struct snd_soc_dapm_context *dapm,
 			 const struct snd_soc_dapm_widget *widget)
-- 
2.39.5

[PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

Use the previously parsed DisCo information from ACPI to create DAPM
widgets and routes representing a SDCA Function. For the most part SDCA
maps well to the DAPM abstractions.

The primary point of interest is the SDCA Power Domain Entities
(PDEs), which actually control the power status of the device. Whilst
these PDEs are the primary widgets the other parts of the SDCA graph
are added to maintain a consistency with the hardware abstract,
and allow routing to take effect. As for the PDEs themselves the
code currently only handle PS0 and PS3 (basically on and off),
the two intermediate power states are not commonly used and don't
map well to ASoC/DAPM.

Other minor points of slightly complexity include, the Group Entities
(GEs) these set the value of several other controls, typically
Selector Units (SUs) for enabling a cetain jack configuration. Multiple
SUs being controlled by a GE are easily modelled creating a single
control and sharing it among the controlled muxes.

SDCA also has a slight habit of having fully connected paths, relying
more on activating the PDEs to enable functionality. This doesn't
map quite so perfectly to DAPM which considers the path a reason to
power the PDE. Whilst in the current specification Mixer Units are
defined as fixed-function, in DAPM we create a virtual control for
each input (which defaults to connected). This allows paths to be
connected/disconnected, providing a more ASoC style approach to
managing the power. PIN_SWITCHs will also be added for non-dataport
terminal entities in a later patch along with the other ALSA controls,
providing greater flexibility in power management.

A top level helper sdca_asoc_populate_component() is exported that
counts and allocates everything, however, the intermediate counting and
population functions are also exported. This will allow end drivers to
do allocation and add custom handling, which is probably fairly likely
for the early SDCA devices.

Clock muxes are currently not fully supported, so some future work will
also be required there.

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

Changes sinve v4:
 - Remove unnecessary MODULE_LICENSE() and MODULE_DESCRIPTION() macros.
 - Move the error message into selector_find_control to better match
   the other search functions.

 include/sound/sdca_asoc.h     |  30 ++
 include/sound/sdca_function.h |  36 ++
 sound/soc/sdca/Makefile       |   2 +-
 sound/soc/sdca/sdca_asoc.c    | 839 ++++++++++++++++++++++++++++++++++
 4 files changed, 906 insertions(+), 1 deletion(-)
 create mode 100644 include/sound/sdca_asoc.h
 create mode 100644 sound/soc/sdca/sdca_asoc.c

diff --git a/include/sound/sdca_asoc.h b/include/sound/sdca_asoc.h
new file mode 100644
index 0000000000000..414d461b6fc4a
--- /dev/null
+++ b/include/sound/sdca_asoc.h
@@ -0,0 +1,30 @@
+/* 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_ASOC_H__
+#define __SDCA_ASOC_H__
+
+struct device;
+struct sdca_function_data;
+struct snd_soc_component_driver;
+struct snd_soc_dapm_route;
+struct snd_soc_dapm_widget;
+
+int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
+			      int *num_widgets, int *num_routes);
+
+int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
+			    struct snd_soc_dapm_widget *widgets,
+			    struct snd_soc_dapm_route *routes);
+
+int sdca_asoc_populate_component(struct device *dev,
+				 struct sdca_function_data *function,
+				 struct snd_soc_component_driver *component_drv);
+
+#endif // __SDCA_ASOC_H__
diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 253654568a41e..83fedc39cf714 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -257,6 +257,14 @@ enum sdca_pde_controls {
 	SDCA_CTL_PDE_ACTUAL_PS				= 0x10,
 };
 
+/**
+ * enum sdca_requested_ps_range - Column definitions for Requested PS
+ */
+enum sdca_requested_ps_range {
+	SDCA_REQUESTED_PS_STATE				= 0,
+	SDCA_REQUESTED_PS_NCOLS				= 1,
+};
+
 /**
  * enum sdca_ge_controls - SDCA Controls for Group Unit
  *
@@ -268,6 +276,15 @@ enum sdca_ge_controls {
 	SDCA_CTL_GE_DETECTED_MODE			= 0x02,
 };
 
+/**
+ * enum sdca_selected_mode_range - Column definitions for Selected Mode
+ */
+enum sdca_selected_mode_range {
+	SDCA_SELECTED_MODE_INDEX			= 0,
+	SDCA_SELECTED_MODE_TERM_TYPE			= 1,
+	SDCA_SELECTED_MODE_NCOLS			= 2,
+};
+
 /**
  * enum sdca_spe_controls - SDCA Controls for Security & Privacy Unit
  *
@@ -773,6 +790,25 @@ enum sdca_terminal_type {
 	SDCA_TERM_TYPE_PRIVACY_INDICATORS		= 0x747,
 };
 
+#define SDCA_TERM_TYPE_LINEIN_STEREO_NAME		"LineIn Stereo"
+#define SDCA_TERM_TYPE_LINEIN_FRONT_LR_NAME		"LineIn Front-LR"
+#define SDCA_TERM_TYPE_LINEIN_CENTER_LFE_NAME		"LineIn Center-LFE"
+#define SDCA_TERM_TYPE_LINEIN_SURROUND_LR_NAME		"LineIn Surround-LR"
+#define SDCA_TERM_TYPE_LINEIN_REAR_LR_NAME		"LineIn Rear-LR"
+#define SDCA_TERM_TYPE_LINEOUT_STEREO_NAME		"LineOut Stereo"
+#define SDCA_TERM_TYPE_LINEOUT_FRONT_LR_NAME		"LineOut Front-LR"
+#define SDCA_TERM_TYPE_LINEOUT_CENTER_LFE_NAME		"LineOut Center-LFE"
+#define SDCA_TERM_TYPE_LINEOUT_SURROUND_LR_NAME		"LineOut Surround-LR"
+#define SDCA_TERM_TYPE_LINEOUT_REAR_LR_NAME		"LineOut Rear-LR"
+#define SDCA_TERM_TYPE_MIC_JACK_NAME			"Microphone"
+#define SDCA_TERM_TYPE_STEREO_JACK_NAME			"Speaker Stereo"
+#define SDCA_TERM_TYPE_FRONT_LR_JACK_NAME		"Speaker Front-LR"
+#define SDCA_TERM_TYPE_CENTER_LFE_JACK_NAME		"Speaker Center-LFE"
+#define SDCA_TERM_TYPE_SURROUND_LR_JACK_NAME		"Speaker Surround-LR"
+#define SDCA_TERM_TYPE_REAR_LR_JACK_NAME		"Speaker Rear-LR"
+#define SDCA_TERM_TYPE_HEADPHONE_JACK_NAME		"Headphone"
+#define SDCA_TERM_TYPE_HEADSET_JACK_NAME		"Headset"
+
 /**
  * enum sdca_connector_type - SDCA Connector Types
  *
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index dddc3e6942569..53344f108ca67 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
-snd-soc-sdca-y	:= sdca_functions.o sdca_device.o sdca_regmap.o
+snd-soc-sdca-y	:= sdca_functions.o sdca_device.o sdca_regmap.o sdca_asoc.o
 
 obj-$(CONFIG_SND_SOC_SDCA)	+= snd-soc-sdca.o
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
new file mode 100644
index 0000000000000..a0c1eb99e97b4
--- /dev/null
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -0,0 +1,839 @@
+// 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/bitmap.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <linux/string_helpers.h>
+#include <sound/control.h>
+#include <sound/sdca.h>
+#include <sound/sdca_asoc.h>
+#include <sound/sdca_function.h>
+#include <sound/soc.h>
+#include <sound/soc-component.h>
+#include <sound/soc-dapm.h>
+
+static struct sdca_control *selector_find_control(struct device *dev,
+						  struct sdca_entity *entity,
+						  const int sel)
+{
+	int i;
+
+	for (i = 0; i < entity->num_controls; i++) {
+		struct sdca_control *control = &entity->controls[i];
+
+		if (control->sel == sel)
+			return control;
+	}
+
+	dev_err(dev, "%s: control %#x: missing\n", entity->label, sel);
+	return NULL;
+}
+
+static struct sdca_control_range *control_find_range(struct device *dev,
+						     struct sdca_entity *entity,
+						     struct sdca_control *control,
+						     int cols, int rows)
+{
+	struct sdca_control_range *range = &control->range;
+
+	if ((cols && range->cols != cols) || (rows && range->rows != rows) ||
+	    !range->data) {
+		dev_err(dev, "%s: control %#x: ranges invalid (%d,%d)\n",
+			entity->label, control->sel, range->cols, range->rows);
+		return NULL;
+	}
+
+	return range;
+}
+
+static struct sdca_control_range *selector_find_range(struct device *dev,
+						      struct sdca_entity *entity,
+						      int sel, int cols, int rows)
+{
+	struct sdca_control *control;
+
+	control = selector_find_control(dev, entity, sel);
+	if (!control)
+		return NULL;
+
+	return control_find_range(dev, entity, control, cols, rows);
+}
+
+/**
+ * sdca_asoc_count_component - count the various component parts
+ * @function: Pointer to the Function information.
+ * @num_widgets: Output integer pointer, will be filled with the
+ * required number of DAPM widgets for the Function.
+ * @num_routes: Output integer pointer, will be filled with the
+ * required number of DAPM routes for the Function.
+ *
+ * This function counts various things within the SDCA Function such
+ * that the calling driver can allocate appropriate space before
+ * calling the appropriate population functions.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
+			      int *num_widgets, int *num_routes)
+{
+	int i;
+
+	*num_widgets = function->num_entities - 1;
+	*num_routes = 0;
+
+	for (i = 0; i < function->num_entities - 1; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		switch (entity->type) {
+		case SDCA_ENTITY_TYPE_IT:
+		case SDCA_ENTITY_TYPE_OT:
+			*num_routes += !!entity->iot.clock;
+			*num_routes += !!entity->iot.is_dataport;
+			break;
+		case SDCA_ENTITY_TYPE_PDE:
+			*num_routes += entity->pde.num_managed;
+			break;
+		default:
+			break;
+		}
+
+		*num_routes += entity->num_sources;
+
+		if (entity->group)
+			(*num_routes)++;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_asoc_count_component, "SND_SOC_SDCA");
+
+static const char *get_terminal_name(enum sdca_terminal_type type)
+{
+	switch (type) {
+	case SDCA_TERM_TYPE_LINEIN_STEREO:
+		return SDCA_TERM_TYPE_LINEIN_STEREO_NAME;
+	case SDCA_TERM_TYPE_LINEIN_FRONT_LR:
+		return SDCA_TERM_TYPE_LINEIN_FRONT_LR_NAME;
+	case SDCA_TERM_TYPE_LINEIN_CENTER_LFE:
+		return SDCA_TERM_TYPE_LINEIN_CENTER_LFE_NAME;
+	case SDCA_TERM_TYPE_LINEIN_SURROUND_LR:
+		return SDCA_TERM_TYPE_LINEIN_SURROUND_LR_NAME;
+	case SDCA_TERM_TYPE_LINEIN_REAR_LR:
+		return SDCA_TERM_TYPE_LINEIN_REAR_LR_NAME;
+	case SDCA_TERM_TYPE_LINEOUT_STEREO:
+		return SDCA_TERM_TYPE_LINEOUT_STEREO_NAME;
+	case SDCA_TERM_TYPE_LINEOUT_FRONT_LR:
+		return SDCA_TERM_TYPE_LINEOUT_FRONT_LR_NAME;
+	case SDCA_TERM_TYPE_LINEOUT_CENTER_LFE:
+		return SDCA_TERM_TYPE_LINEOUT_CENTER_LFE_NAME;
+	case SDCA_TERM_TYPE_LINEOUT_SURROUND_LR:
+		return SDCA_TERM_TYPE_LINEOUT_SURROUND_LR_NAME;
+	case SDCA_TERM_TYPE_LINEOUT_REAR_LR:
+		return SDCA_TERM_TYPE_LINEOUT_REAR_LR_NAME;
+	case SDCA_TERM_TYPE_MIC_JACK:
+		return SDCA_TERM_TYPE_MIC_JACK_NAME;
+	case SDCA_TERM_TYPE_STEREO_JACK:
+		return SDCA_TERM_TYPE_STEREO_JACK_NAME;
+	case SDCA_TERM_TYPE_FRONT_LR_JACK:
+		return SDCA_TERM_TYPE_FRONT_LR_JACK_NAME;
+	case SDCA_TERM_TYPE_CENTER_LFE_JACK:
+		return SDCA_TERM_TYPE_CENTER_LFE_JACK_NAME;
+	case SDCA_TERM_TYPE_SURROUND_LR_JACK:
+		return SDCA_TERM_TYPE_SURROUND_LR_JACK_NAME;
+	case SDCA_TERM_TYPE_REAR_LR_JACK:
+		return SDCA_TERM_TYPE_REAR_LR_JACK_NAME;
+	case SDCA_TERM_TYPE_HEADPHONE_JACK:
+		return SDCA_TERM_TYPE_HEADPHONE_JACK_NAME;
+	case SDCA_TERM_TYPE_HEADSET_JACK:
+		return SDCA_TERM_TYPE_HEADSET_JACK_NAME;
+	default:
+		return NULL;
+	}
+}
+
+static int entity_early_parse_ge(struct device *dev,
+				 struct sdca_function_data *function,
+				 struct sdca_entity *entity)
+{
+	struct sdca_control_range *range;
+	struct sdca_control *control;
+	struct snd_kcontrol_new *kctl;
+	struct soc_enum *soc_enum;
+	const char *control_name;
+	unsigned int *values;
+	const char **texts;
+	int i;
+
+	control = selector_find_control(dev, entity, SDCA_CTL_GE_SELECTED_MODE);
+	if (!control)
+		return -EINVAL;
+
+	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
+		dev_warn(dev, "%s: unexpected access layer: %x\n",
+			 entity->label, control->layers);
+
+	range = control_find_range(dev, entity, control, SDCA_SELECTED_MODE_NCOLS, 0);
+	if (!range)
+		return -EINVAL;
+
+	control_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
+				      entity->label, control->label);
+	if (!control_name)
+		return -ENOMEM;
+
+	kctl = devm_kmalloc(dev, sizeof(*kctl), GFP_KERNEL);
+	if (!kctl)
+		return -ENOMEM;
+
+	soc_enum = devm_kmalloc(dev, sizeof(*soc_enum), GFP_KERNEL);
+	if (!soc_enum)
+		return -ENOMEM;
+
+	texts = devm_kcalloc(dev, range->rows + 3, sizeof(*texts), GFP_KERNEL);
+	if (!texts)
+		return -ENOMEM;
+
+	values = devm_kcalloc(dev, range->rows + 3, sizeof(*values), GFP_KERNEL);
+	if (!values)
+		return -ENOMEM;
+
+	texts[0] = "No Jack";
+	texts[1] = "Jack Unknown";
+	texts[2] = "Detection in Progress";
+	values[0] = 0;
+	values[1] = 1;
+	values[2] = 2;
+	for (i = 0; i < range->rows; i++) {
+		enum sdca_terminal_type type;
+
+		type = sdca_range(range, SDCA_SELECTED_MODE_TERM_TYPE, i);
+
+		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);
+		texts[i + 3] = get_terminal_name(type);
+		if (!texts[i + 3]) {
+			dev_err(dev, "%s: unrecognised terminal type: %#x\n",
+				entity->label, type);
+			return -EINVAL;
+		}
+	}
+
+	soc_enum->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
+	soc_enum->items = range->rows + 3;
+	soc_enum->mask = roundup_pow_of_two(soc_enum->items) - 1;
+	soc_enum->texts = texts;
+	soc_enum->values = values;
+
+	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kctl->name = control_name;
+	kctl->info = snd_soc_info_enum_double;
+	kctl->get = snd_soc_dapm_get_enum_double;
+	kctl->put = snd_soc_dapm_put_enum_double;
+	kctl->private_value = (unsigned long)soc_enum;
+
+	entity->ge.kctl = kctl;
+
+	return 0;
+}
+
+static void add_route(struct snd_soc_dapm_route **route, const char *sink,
+		      const char *control, const char *source)
+{
+	(*route)->sink = sink;
+	(*route)->control = control;
+	(*route)->source = source;
+	(*route)++;
+}
+
+static int entity_parse_simple(struct device *dev,
+			       struct sdca_function_data *function,
+			       struct sdca_entity *entity,
+			       struct snd_soc_dapm_widget **widget,
+			       struct snd_soc_dapm_route **route,
+			       enum snd_soc_dapm_type id)
+{
+	int i;
+
+	(*widget)->id = id;
+	(*widget)++;
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, NULL, entity->sources[i]->label);
+
+	return 0;
+}
+
+static int entity_parse_it(struct device *dev,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct snd_soc_dapm_widget **widget,
+			   struct snd_soc_dapm_route **route)
+{
+	int i;
+
+	if (entity->iot.is_dataport) {
+		const char *aif_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
+						      entity->label, "Playback");
+		if (!aif_name)
+			return -ENOMEM;
+
+		(*widget)->id = snd_soc_dapm_aif_in;
+
+		add_route(route, entity->label, NULL, aif_name);
+	} else {
+		(*widget)->id = snd_soc_dapm_mic;
+	}
+
+	if (entity->iot.clock)
+		add_route(route, entity->label, NULL, entity->iot.clock->label);
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, NULL, entity->sources[i]->label);
+
+	(*widget)++;
+
+	return 0;
+}
+
+static int entity_parse_ot(struct device *dev,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct snd_soc_dapm_widget **widget,
+			   struct snd_soc_dapm_route **route)
+{
+	int i;
+
+	if (entity->iot.is_dataport) {
+		const char *aif_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
+						      entity->label, "Capture");
+		if (!aif_name)
+			return -ENOMEM;
+
+		(*widget)->id = snd_soc_dapm_aif_out;
+
+		add_route(route, aif_name, NULL, entity->label);
+	} else {
+		(*widget)->id = snd_soc_dapm_spk;
+	}
+
+	if (entity->iot.clock)
+		add_route(route, entity->label, NULL, entity->iot.clock->label);
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, NULL, entity->sources[i]->label);
+
+	(*widget)++;
+
+	return 0;
+}
+
+static int entity_pde_event(struct snd_soc_dapm_widget *widget,
+			    struct snd_kcontrol *kctl, int event)
+{
+	struct snd_soc_component *component = widget->dapm->component;
+	struct sdca_entity *entity = widget->priv;
+	static const int poll_us = 10000;
+	int polls = 1;
+	unsigned int reg, val;
+	int from, to, i;
+	int ret;
+
+	if (!component)
+		return -EIO;
+
+	switch (event) {
+	case SND_SOC_DAPM_POST_PMD:
+		from = widget->on_val;
+		to = widget->off_val;
+		break;
+	case SND_SOC_DAPM_POST_PMU:
+		from = widget->off_val;
+		to = widget->on_val;
+		break;
+	}
+
+	for (i = 0; i < entity->pde.num_max_delay; i++) {
+		struct sdca_pde_delay *delay = &entity->pde.max_delay[i];
+
+		if (delay->from_ps == from && delay->to_ps == to) {
+			polls = max(polls, delay->us / poll_us);
+			break;
+		}
+	}
+
+	reg = SDW_SDCA_CTL(SDW_SDCA_CTL_FUNC(widget->reg),
+			   SDW_SDCA_CTL_ENT(widget->reg),
+			   SDCA_CTL_PDE_ACTUAL_PS, 0);
+
+	for (i = 0; i < polls; i++) {
+		if (i)
+			fsleep(poll_us);
+
+		ret = regmap_read(component->regmap, reg, &val);
+		if (ret)
+			return ret;
+		else if (val == to)
+			return 0;
+	}
+
+	dev_err(component->dev, "%s: power transition failed: %x\n",
+		entity->label, val);
+	return -ETIMEDOUT;
+}
+
+static int entity_parse_pde(struct device *dev,
+			    struct sdca_function_data *function,
+			    struct sdca_entity *entity,
+			    struct snd_soc_dapm_widget **widget,
+			    struct snd_soc_dapm_route **route)
+{
+	unsigned int target = (1 << SDCA_PDE_PS0) | (1 << SDCA_PDE_PS3);
+	struct sdca_control_range *range;
+	struct sdca_control *control;
+	unsigned int mask = 0;
+	int i;
+
+	control = selector_find_control(dev, entity, SDCA_CTL_PDE_REQUESTED_PS);
+	if (!control)
+		return -EINVAL;
+
+	/* Power should only be controlled by the driver */
+	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
+		dev_warn(dev, "%s: unexpected access layer: %x\n",
+			 entity->label, control->layers);
+
+	range = control_find_range(dev, entity, control, SDCA_REQUESTED_PS_NCOLS, 0);
+	if (!range)
+		return -EINVAL;
+
+	for (i = 0; i < range->rows; i++)
+		mask |= 1 << sdca_range(range, SDCA_REQUESTED_PS_STATE, i);
+
+	if ((mask & target) != target) {
+		dev_err(dev, "%s: power control missing states\n", entity->label);
+		return -EINVAL;
+	}
+
+	(*widget)->id = snd_soc_dapm_supply;
+	(*widget)->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
+	(*widget)->mask = GENMASK(control->nbits - 1, 0);
+	(*widget)->on_val = SDCA_PDE_PS0;
+	(*widget)->off_val = SDCA_PDE_PS3;
+	(*widget)->event_flags = SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD;
+	(*widget)->event = entity_pde_event;
+	(*widget)->priv = entity;
+	(*widget)++;
+
+	for (i = 0; i < entity->pde.num_managed; i++)
+		add_route(route, entity->pde.managed[i]->label, NULL, entity->label);
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, NULL, entity->sources[i]->label);
+
+	return 0;
+}
+
+/* Device selector units are controlled through a group entity */
+static int entity_parse_su_device(struct device *dev,
+				  struct sdca_function_data *function,
+				  struct sdca_entity *entity,
+				  struct snd_soc_dapm_widget **widget,
+				  struct snd_soc_dapm_route **route)
+{
+	struct sdca_control_range *range;
+	int num_routes = 0;
+	int i, j;
+
+	if (!entity->group) {
+		dev_err(dev, "%s: device selector unit missing group\n", entity->label);
+		return -EINVAL;
+	}
+
+	range = selector_find_range(dev, entity->group, SDCA_CTL_GE_SELECTED_MODE,
+				    SDCA_SELECTED_MODE_NCOLS, 0);
+	if (!range)
+		return -EINVAL;
+
+	(*widget)->id = snd_soc_dapm_mux;
+	(*widget)->kcontrol_news = entity->group->ge.kctl;
+	(*widget)->num_kcontrols = 1;
+	(*widget)++;
+
+	for (i = 0; i < entity->group->ge.num_modes; i++) {
+		struct sdca_ge_mode *mode = &entity->group->ge.modes[i];
+
+		for (j = 0; j < mode->num_controls; j++) {
+			struct sdca_ge_control *affected = &mode->controls[j];
+			int term;
+
+			if (affected->id != entity->id ||
+			    affected->sel != SDCA_CTL_SU_SELECTOR ||
+			    !affected->val)
+				continue;
+
+			if (affected->val - 1 >= entity->num_sources) {
+				dev_err(dev, "%s: bad control value: %#x\n",
+					entity->label, affected->val);
+				return -EINVAL;
+			}
+
+			if (++num_routes > entity->num_sources) {
+				dev_err(dev, "%s: too many input routes\n", entity->label);
+				return -EINVAL;
+			}
+
+			term = sdca_range_search(range, SDCA_SELECTED_MODE_INDEX,
+						 mode->val, SDCA_SELECTED_MODE_TERM_TYPE);
+			if (!term) {
+				dev_err(dev, "%s: mode not found: %#x\n",
+					entity->label, mode->val);
+				return -EINVAL;
+			}
+
+			add_route(route, entity->label, get_terminal_name(term),
+				  entity->sources[affected->val - 1]->label);
+		}
+	}
+
+	return 0;
+}
+
+/* Class selector units will be exported as an ALSA control */
+static int entity_parse_su_class(struct device *dev,
+				 struct sdca_function_data *function,
+				 struct sdca_entity *entity,
+				 struct sdca_control *control,
+				 struct snd_soc_dapm_widget **widget,
+				 struct snd_soc_dapm_route **route)
+{
+	struct snd_kcontrol_new *kctl;
+	struct soc_enum *soc_enum;
+	const char **texts;
+	int i;
+
+	kctl = devm_kmalloc(dev, sizeof(*kctl), GFP_KERNEL);
+	if (!kctl)
+		return -ENOMEM;
+
+	soc_enum = devm_kmalloc(dev, sizeof(*soc_enum), GFP_KERNEL);
+	if (!soc_enum)
+		return -ENOMEM;
+
+	texts = devm_kcalloc(dev, entity->num_sources + 1, sizeof(*texts), GFP_KERNEL);
+	if (!texts)
+		return -ENOMEM;
+
+	texts[0] = "No Signal";
+	for (i = 0; i < entity->num_sources; i++)
+		texts[i + 1] = entity->sources[i]->label;
+
+	soc_enum->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
+	soc_enum->items = entity->num_sources + 1;
+	soc_enum->mask = roundup_pow_of_two(soc_enum->items) - 1;
+	soc_enum->texts = texts;
+
+	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	kctl->name = "Route";
+	kctl->info = snd_soc_info_enum_double;
+	kctl->get = snd_soc_dapm_get_enum_double;
+	kctl->put = snd_soc_dapm_put_enum_double;
+	kctl->private_value = (unsigned long)soc_enum;
+
+	(*widget)->id = snd_soc_dapm_mux;
+	(*widget)->kcontrol_news = kctl;
+	(*widget)->num_kcontrols = 1;
+	(*widget)++;
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, texts[i + 1], entity->sources[i]->label);
+
+	return 0;
+}
+
+static int entity_parse_su(struct device *dev,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct snd_soc_dapm_widget **widget,
+			   struct snd_soc_dapm_route **route)
+{
+	struct sdca_control *control;
+
+	if (!entity->num_sources) {
+		dev_err(dev, "%s: selector with no inputs\n", entity->label);
+		return -EINVAL;
+	}
+
+	control = selector_find_control(dev, entity, SDCA_CTL_SU_SELECTOR);
+	if (!control)
+		return -EINVAL;
+
+	if (control->layers == SDCA_ACCESS_LAYER_DEVICE)
+		return entity_parse_su_device(dev, function, entity, widget, route);
+
+	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
+		dev_warn(dev, "%s: unexpected access layer: %x\n",
+			 entity->label, control->layers);
+
+	return entity_parse_su_class(dev, function, entity, control, widget, route);
+}
+
+static int entity_parse_mu(struct device *dev,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct snd_soc_dapm_widget **widget,
+			   struct snd_soc_dapm_route **route)
+{
+	struct sdca_control *control;
+	struct snd_kcontrol_new *kctl;
+	int cn;
+	int i;
+
+	if (!entity->num_sources) {
+		dev_err(dev, "%s: selector 1 or more inputs\n", entity->label);
+		return -EINVAL;
+	}
+
+	control = selector_find_control(dev, entity, SDCA_CTL_MU_MIXER);
+	if (!control)
+		return -EINVAL;
+
+	/* MU control should be through DAPM */
+	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
+		dev_warn(dev, "%s: unexpected access layer: %x\n",
+			 entity->label, control->layers);
+
+	if (entity->num_sources != hweight64(control->cn_list)) {
+		dev_err(dev, "%s: mismatched control and sources\n", entity->label);
+		return -EINVAL;
+	}
+
+	kctl = devm_kcalloc(dev, entity->num_sources, sizeof(*kctl), GFP_KERNEL);
+	if (!kctl)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_set_bit(cn, (unsigned long *)&control->cn_list,
+			 BITS_PER_TYPE(control->cn_list)) {
+		const char *control_name;
+		struct soc_mixer_control *mc;
+
+		control_name = devm_kasprintf(dev, GFP_KERNEL, "%s %d",
+					      control->label, i + 1);
+		if (!control_name)
+			return -ENOMEM;
+
+		mc = devm_kmalloc(dev, sizeof(*mc), GFP_KERNEL);
+		if (!mc)
+			return -ENOMEM;
+
+		mc->reg = SND_SOC_NOPM;
+		mc->rreg = SND_SOC_NOPM;
+		mc->invert = 1; // Ensure default is connected
+		mc->min = 0;
+		mc->max = 1;
+
+		kctl[i].name = control_name;
+		kctl[i].private_value = (unsigned long)mc;
+		kctl[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+		kctl[i].info = snd_soc_info_volsw;
+		kctl[i].get = snd_soc_dapm_get_volsw;
+		kctl[i].put = snd_soc_dapm_put_volsw;
+		i++;
+	}
+
+	(*widget)->id = snd_soc_dapm_mixer;
+	(*widget)->kcontrol_news = kctl;
+	(*widget)->num_kcontrols = entity->num_sources;
+	(*widget)++;
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, kctl[i].name, entity->sources[i]->label);
+
+	return 0;
+}
+
+static int entity_cs_event(struct snd_soc_dapm_widget *widget,
+			   struct snd_kcontrol *kctl, int event)
+{
+	struct snd_soc_component *component = widget->dapm->component;
+	struct sdca_entity *entity = widget->priv;
+
+	if (!component)
+		return -EIO;
+
+	if (entity->cs.max_delay)
+		fsleep(entity->cs.max_delay);
+
+	return 0;
+}
+
+static int entity_parse_cs(struct device *dev,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct snd_soc_dapm_widget **widget,
+			   struct snd_soc_dapm_route **route)
+{
+	int i;
+
+	(*widget)->id = snd_soc_dapm_supply;
+	(*widget)->subseq = 1; /* Ensure these run after PDEs */
+	(*widget)->event_flags = SND_SOC_DAPM_POST_PMU;
+	(*widget)->event = entity_cs_event;
+	(*widget)->priv = entity;
+	(*widget)++;
+
+	for (i = 0; i < entity->num_sources; i++)
+		add_route(route, entity->label, NULL, entity->sources[i]->label);
+
+	return 0;
+}
+
+/**
+ * sdca_asoc_populate_dapm - fill in arrays of DAPM widgets and routes
+ * @dev: Pointer to the device against which allocations will be done.
+ * @function: Pointer to the Function information.
+ * @widget: Array of DAPM widgets to be populated.
+ * @route: Array of DAPM routes to be populated.
+ *
+ * This function populates arrays of DAPM widgets and routes from the
+ * DisCo information for a particular SDCA Function. Typically,
+ * snd_soc_asoc_count_component will be used to allocate appropriately
+ * sized arrays before calling this function.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
+			    struct snd_soc_dapm_widget *widget,
+			    struct snd_soc_dapm_route *route)
+{
+	int ret;
+	int i;
+
+	/* Some entities need to add controls referenced by other entities */
+	for (i = 0; i < function->num_entities - 1; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		switch (entity->type) {
+		case SDCA_ENTITY_TYPE_GE:
+			ret = entity_early_parse_ge(dev, function, entity);
+			if (ret)
+				return ret;
+			break;
+		default:
+			break;
+		}
+	}
+
+	for (i = 0; i < function->num_entities - 1; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		widget->name = entity->label;
+		widget->reg = SND_SOC_NOPM;
+
+		switch (entity->type) {
+		case SDCA_ENTITY_TYPE_IT:
+			ret = entity_parse_it(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_OT:
+			ret = entity_parse_ot(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_PDE:
+			ret = entity_parse_pde(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_SU:
+			ret = entity_parse_su(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_MU:
+			ret = entity_parse_mu(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_CS:
+			ret = entity_parse_cs(dev, function, entity, &widget, &route);
+			break;
+		case SDCA_ENTITY_TYPE_CX:
+			/*
+			 * FIXME: For now we will just treat these as a supply,
+			 * meaning all options are enabled.
+			 */
+			dev_warn(dev, "%s: clock selectors not fully supported yet\n",
+				 entity->label);
+			ret = entity_parse_simple(dev, function, entity, &widget,
+						  &route, snd_soc_dapm_supply);
+			break;
+		case SDCA_ENTITY_TYPE_TG:
+			ret = entity_parse_simple(dev, function, entity, &widget,
+						  &route, snd_soc_dapm_siggen);
+			break;
+		case SDCA_ENTITY_TYPE_GE:
+			ret = entity_parse_simple(dev, function, entity, &widget,
+						  &route, snd_soc_dapm_supply);
+			break;
+		default:
+			ret = entity_parse_simple(dev, function, entity, &widget,
+						  &route, snd_soc_dapm_pga);
+			break;
+		}
+		if (ret)
+			return ret;
+
+		if (entity->group)
+			add_route(&route, entity->label, NULL, entity->group->label);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_asoc_populate_dapm, "SND_SOC_SDCA");
+
+/**
+ * sdca_asoc_populate_component - fill in a component driver for a Function
+ * @dev: Pointer to the device against which allocations will be done.
+ * @function: Pointer to the Function information.
+ * @copmonent_drv: Pointer to the component driver to be populated.
+ *
+ * This function populates a snd_soc_component_driver structure based
+ * on the DisCo information for a particular SDCA Function. It does
+ * all allocation internally.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_asoc_populate_component(struct device *dev,
+				 struct sdca_function_data *function,
+				 struct snd_soc_component_driver *component_drv)
+{
+	struct snd_soc_dapm_widget *widgets;
+	struct snd_soc_dapm_route *routes;
+	int num_widgets, num_routes;
+	int ret;
+
+	ret = sdca_asoc_count_component(dev, function, &num_widgets, &num_routes);
+	if (ret)
+		return ret;
+
+	widgets = devm_kcalloc(dev, num_widgets, sizeof(*widgets), GFP_KERNEL);
+	if (!widgets)
+		return -ENOMEM;
+
+	routes = devm_kcalloc(dev, num_routes, sizeof(*routes), GFP_KERNEL);
+	if (!routes)
+		return -ENOMEM;
+
+	ret = sdca_asoc_populate_dapm(dev, function, widgets, routes);
+	if (ret)
+		return ret;
+
+	component_drv->dapm_widgets = widgets;
+	component_drv->num_dapm_widgets = num_widgets;
+	component_drv->dapm_routes = routes;
+	component_drv->num_dapm_routes = num_routes;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_asoc_populate_component, "SND_SOC_SDCA");
-- 
2.39.5

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Pierre-Louis Bossart]

On 5/12/25 14:42, Charles Keepax wrote:
> Use the previously parsed DisCo information from ACPI to create DAPM
> widgets and routes representing a SDCA Function. For the most part SDCA
> maps well to the DAPM abstractions.
> 
> The primary point of interest is the SDCA Power Domain Entities
> (PDEs), which actually control the power status of the device. Whilst
> these PDEs are the primary widgets the other parts of the SDCA graph
> are added to maintain a consistency with the hardware abstract,
> and allow routing to take effect. As for the PDEs themselves the
> code currently only handle PS0 and PS3 (basically on and off),
> the two intermediate power states are not commonly used and don't
> map well to ASoC/DAPM.
> 
> Other minor points of slightly complexity include, the Group Entities
> (GEs) these set the value of several other controls, typically
> Selector Units (SUs) for enabling a cetain jack configuration. Multiple
> SUs being controlled by a GE are easily modelled creating a single
> control and sharing it among the controlled muxes.
> 
> SDCA also has a slight habit of having fully connected paths, relying
> more on activating the PDEs to enable functionality. This doesn't
> map quite so perfectly to DAPM which considers the path a reason to
> power the PDE. Whilst in the current specification Mixer Units are
> defined as fixed-function, in DAPM we create a virtual control for
> each input (which defaults to connected). This allows paths to be
> connected/disconnected, providing a more ASoC style approach to
> managing the power. PIN_SWITCHs will also be added for non-dataport
> terminal entities in a later patch along with the other ALSA controls,
> providing greater flexibility in power management.

It's been a while since I reviewed an earlier versions so now I am confused.
- Is the proposal to have a control to change the PDE state directly, as well as a virtual control to describe a path activation? That would lead to more flexibility but also to confusion, e.g. with 'no sound' when the paths are active but the PDE still in PS3.
- Or do those virtual controls manage the PDE state, without userspace interaction? In that case the 'connected' default would higher power consumption until userspace decides which paths it really wants active.
 

> +/**
> + * sdca_asoc_count_component - count the various component parts
> + * @function: Pointer to the Function information.
> + * @num_widgets: Output integer pointer, will be filled with the
> + * required number of DAPM widgets for the Function.
> + * @num_routes: Output integer pointer, will be filled with the
> + * required number of DAPM routes for the Function.
> + *
> + * This function counts various things within the SDCA Function such
> + * that the calling driver can allocate appropriate space before
> + * calling the appropriate population functions.
> + *
> + * Return: Returns zero on success, and a negative error code on failure.
> + */
> +int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
> +			      int *num_widgets, int *num_routes)
> +{
> +	int i;
> +
> +	*num_widgets = function->num_entities - 1;
> +	*num_routes = 0;
> +
> +	for (i = 0; i < function->num_entities - 1; i++) {
> +		struct sdca_entity *entity = &function->entities[i];
> +
> +		switch (entity->type) {
> +		case SDCA_ENTITY_TYPE_IT:
> +		case SDCA_ENTITY_TYPE_OT:
> +			*num_routes += !!entity->iot.clock;
> +			*num_routes += !!entity->iot.is_dataport;

I couldn't follow those two lines. 
Why does the clock play a role, for now it's not really managed?
And the difference between streaming and transducer terminals shouldn't change the accounting of routes?

> +			break;
> +		case SDCA_ENTITY_TYPE_PDE:
> +			*num_routes += entity->pde.num_managed;

same here, isn't there a risk of the same route being counted multiple times?

> +			break;
> +		default:
> +			break;
> +		}
> +
> +		*num_routes += entity->num_sources;
> +
> +		if (entity->group)
> +			(*num_routes)++;

And here as well, in case an entity is controlled by a GE does the number of routes really change? 

I guess the meaning of 'routes' isn't really clear in my head, it's not just data paths but control as well that's taken into account?

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(sdca_asoc_count_component, "SND_SOC_SDCA");

> +static int entity_early_parse_ge(struct device *dev,
> +				 struct sdca_function_data *function,
> +				 struct sdca_entity *entity)
> +{
> +	struct sdca_control_range *range;
> +	struct sdca_control *control;
> +	struct snd_kcontrol_new *kctl;
> +	struct soc_enum *soc_enum;
> +	const char *control_name;
> +	unsigned int *values;
> +	const char **texts;
> +	int i;
> +
> +	control = selector_find_control(dev, entity, SDCA_CTL_GE_SELECTED_MODE);
> +	if (!control)
> +		return -EINVAL;
> +
> +	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
> +		dev_warn(dev, "%s: unexpected access layer: %x\n",
> +			 entity->label, control->layers);
> +
> +	range = control_find_range(dev, entity, control, SDCA_SELECTED_MODE_NCOLS, 0);
> +	if (!range)
> +		return -EINVAL;
> +
> +	control_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
> +				      entity->label, control->label);
> +	if (!control_name)
> +		return -ENOMEM;
> +
> +	kctl = devm_kmalloc(dev, sizeof(*kctl), GFP_KERNEL);
> +	if (!kctl)
> +		return -ENOMEM;
> +
> +	soc_enum = devm_kmalloc(dev, sizeof(*soc_enum), GFP_KERNEL);
> +	if (!soc_enum)
> +		return -ENOMEM;
> +
> +	texts = devm_kcalloc(dev, range->rows + 3, sizeof(*texts), GFP_KERNEL);
> +	if (!texts)
> +		return -ENOMEM;
> +
> +	values = devm_kcalloc(dev, range->rows + 3, sizeof(*values), GFP_KERNEL);
> +	if (!values)
> +		return -ENOMEM;
> +
> +	texts[0] = "No Jack";
> +	texts[1] = "Jack Unknown";
> +	texts[2] = "Detection in Progress";
> +	values[0] = 0;
> +	values[1] = 1;
> +	values[2] = 2;
> +	for (i = 0; i < range->rows; i++) {
> +		enum sdca_terminal_type type;
> +
> +		type = sdca_range(range, SDCA_SELECTED_MODE_TERM_TYPE, i);
> +
> +		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);

Humm, my memory of GEs is that the hardware reports a 'DETECTED_MODE' and then software (kernel and/or userspace) can choose a "SELECTED_MODE". here we only deal with SELECTED_MODE, is this intentional?

> +		texts[i + 3] = get_terminal_name(type);
> +		if (!texts[i + 3]) {
> +			dev_err(dev, "%s: unrecognised terminal type: %#x\n",
> +				entity->label, type);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	soc_enum->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
> +	soc_enum->items = range->rows + 3;
> +	soc_enum->mask = roundup_pow_of_two(soc_enum->items) - 1;
> +	soc_enum->texts = texts;
> +	soc_enum->values = values;
> +
> +	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	kctl->name = control_name;
> +	kctl->info = snd_soc_info_enum_double;
> +	kctl->get = snd_soc_dapm_get_enum_double;
> +	kctl->put = snd_soc_dapm_put_enum_double;
> +	kctl->private_value = (unsigned long)soc_enum;
> +
> +	entity->ge.kctl = kctl;
> +
> +	return 0;
> +}

> +static int entity_parse_it(struct device *dev,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   struct snd_soc_dapm_widget **widget,
> +			   struct snd_soc_dapm_route **route)
> +{
> +	int i;
> +
> +	if (entity->iot.is_dataport) {
> +		const char *aif_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
> +						      entity->label, "Playback");
> +		if (!aif_name)
> +			return -ENOMEM;
> +
> +		(*widget)->id = snd_soc_dapm_aif_in;
> +
> +		add_route(route, entity->label, NULL, aif_name);
> +	} else {
> +		(*widget)->id = snd_soc_dapm_mic;

so for non-dataport terminals there's no route added, but didn't the intro say that there was a virtual control for such terminals? that seems like a route definition if a control can alter the data path, no?

> +	}
> +
> +	if (entity->iot.clock)
> +		add_route(route, entity->label, NULL, entity->iot.clock->label);
> +
> +	for (i = 0; i < entity->num_sources; i++)
> +		add_route(route, entity->label, NULL, entity->sources[i]->label);
> +
> +	(*widget)++;
> +
> +	return 0;
> +}

> +static int entity_pde_event(struct snd_soc_dapm_widget *widget,
> +			    struct snd_kcontrol *kctl, int event)
> +{
> +	struct snd_soc_component *component = widget->dapm->component;
> +	struct sdca_entity *entity = widget->priv;
> +	static const int poll_us = 10000;

This should be a #define, no?

> +	int polls = 1;
> +	unsigned int reg, val;
> +	int from, to, i;
> +	int ret;
> +
> +	if (!component)
> +		return -EIO;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMD:
> +		from = widget->on_val;
> +		to = widget->off_val;
> +		break;
> +	case SND_SOC_DAPM_POST_PMU:
> +		from = widget->off_val;
> +		to = widget->on_val;
> +		break;
> +	}
> +
> +	for (i = 0; i < entity->pde.num_max_delay; i++) {
> +		struct sdca_pde_delay *delay = &entity->pde.max_delay[i];
> +
> +		if (delay->from_ps == from && delay->to_ps == to) {
> +			polls = max(polls, delay->us / poll_us);
> +			break;
> +		}
> +	}
> +
> +	reg = SDW_SDCA_CTL(SDW_SDCA_CTL_FUNC(widget->reg),
> +			   SDW_SDCA_CTL_ENT(widget->reg),
> +			   SDCA_CTL_PDE_ACTUAL_PS, 0);
> +
> +	for (i = 0; i < polls; i++) {
> +		if (i)
> +			fsleep(poll_us);

The logic seems inverted? I would first sleep by the amount of time required for a transition before reading the status register. Here we read it first and then sleep for following iterations. The first read is almost guaranteed to fail.

> +
> +		ret = regmap_read(component->regmap, reg, &val);
> +		if (ret)
> +			return ret;
> +		else if (val == to)
> +			return 0;
> +	}
> +
> +	dev_err(component->dev, "%s: power transition failed: %x\n",
> +		entity->label, val);
> +	return -ETIMEDOUT;
> +}
> +
> +static int entity_parse_pde(struct device *dev,
> +			    struct sdca_function_data *function,
> +			    struct sdca_entity *entity,
> +			    struct snd_soc_dapm_widget **widget,
> +			    struct snd_soc_dapm_route **route)
> +{
> +	unsigned int target = (1 << SDCA_PDE_PS0) | (1 << SDCA_PDE_PS3);
> +	struct sdca_control_range *range;
> +	struct sdca_control *control;
> +	unsigned int mask = 0;
> +	int i;
> +
> +	control = selector_find_control(dev, entity, SDCA_CTL_PDE_REQUESTED_PS);
> +	if (!control)
> +		return -EINVAL;
> +
> +	/* Power should only be controlled by the driver */

is it though? With the virtual controls you were referring to earlier, it's possible that the path is not used and the PDE in PS3. I guess I am still in the dark on what controls the PDE.

> +	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
> +		dev_warn(dev, "%s: unexpected access layer: %x\n",
> +			 entity->label, control->layers);
> +
> +	range = control_find_range(dev, entity, control, SDCA_REQUESTED_PS_NCOLS, 0);
> +	if (!range)
> +		return -EINVAL;
> +
> +	for (i = 0; i < range->rows; i++)
> +		mask |= 1 << sdca_range(range, SDCA_REQUESTED_PS_STATE, i);
> +
> +	if ((mask & target) != target) {
> +		dev_err(dev, "%s: power control missing states\n", entity->label);
> +		return -EINVAL;
> +	}
> +
> +	(*widget)->id = snd_soc_dapm_supply;
> +	(*widget)->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
> +	(*widget)->mask = GENMASK(control->nbits - 1, 0);
> +	(*widget)->on_val = SDCA_PDE_PS0;
> +	(*widget)->off_val = SDCA_PDE_PS3;
> +	(*widget)->event_flags = SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD;
> +	(*widget)->event = entity_pde_event;
> +	(*widget)->priv = entity;
> +	(*widget)++;
> +
> +	for (i = 0; i < entity->pde.num_managed; i++)
> +		add_route(route, entity->pde.managed[i]->label, NULL, entity->label);
> +
> +	for (i = 0; i < entity->num_sources; i++)
> +		add_route(route, entity->label, NULL, entity->sources[i]->label);
> +
> +	return 0;
> +}

> +/* Class selector units will be exported as an ALSA control */
> +static int entity_parse_su_class(struct device *dev,
> +				 struct sdca_function_data *function,
> +				 struct sdca_entity *entity,
> +				 struct sdca_control *control,
> +				 struct snd_soc_dapm_widget **widget,
> +				 struct snd_soc_dapm_route **route)
> +{
> +	struct snd_kcontrol_new *kctl;
> +	struct soc_enum *soc_enum;
> +	const char **texts;
> +	int i;
> +
> +	kctl = devm_kmalloc(dev, sizeof(*kctl), GFP_KERNEL);
> +	if (!kctl)
> +		return -ENOMEM;
> +
> +	soc_enum = devm_kmalloc(dev, sizeof(*soc_enum), GFP_KERNEL);
> +	if (!soc_enum)
> +		return -ENOMEM;
> +
> +	texts = devm_kcalloc(dev, entity->num_sources + 1, sizeof(*texts), GFP_KERNEL);
> +	if (!texts)
> +		return -ENOMEM;
> +
> +	texts[0] = "No Signal";
> +	for (i = 0; i < entity->num_sources; i++)
> +		texts[i + 1] = entity->sources[i]->label;
> +
> +	soc_enum->reg = SDW_SDCA_CTL(function->desc->adr, entity->id, control->sel, 0);
> +	soc_enum->items = entity->num_sources + 1;
> +	soc_enum->mask = roundup_pow_of_two(soc_enum->items) - 1;
> +	soc_enum->texts = texts;
> +
> +	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	kctl->name = "Route";
> +	kctl->info = snd_soc_info_enum_double;
> +	kctl->get = snd_soc_dapm_get_enum_double;
> +	kctl->put = snd_soc_dapm_put_enum_double;
> +	kctl->private_value = (unsigned long)soc_enum;

Can you remind me in which cases we have a Selector Unit not controlled by a GE, and what userspace would do with such kcontrols?

> +
> +	(*widget)->id = snd_soc_dapm_mux;
> +	(*widget)->kcontrol_news = kctl;
> +	(*widget)->num_kcontrols = 1;
> +	(*widget)++;
> +
> +	for (i = 0; i < entity->num_sources; i++)
> +		add_route(route, entity->label, texts[i + 1], entity->sources[i]->label);
> +
> +	return 0;
> +}

> +static int entity_parse_mu(struct device *dev,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   struct snd_soc_dapm_widget **widget,
> +			   struct snd_soc_dapm_route **route)
> +{
> +	struct sdca_control *control;
> +	struct snd_kcontrol_new *kctl;
> +	int cn;
> +	int i;
> +
> +	if (!entity->num_sources) {
> +		dev_err(dev, "%s: selector 1 or more inputs\n", entity->label);
> +		return -EINVAL;
> +	}
> +
> +	control = selector_find_control(dev, entity, SDCA_CTL_MU_MIXER);
> +	if (!control)
> +		return -EINVAL;
> +
> +	/* MU control should be through DAPM */
> +	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
> +		dev_warn(dev, "%s: unexpected access layer: %x\n",
> +			 entity->label, control->layers);

isn't this an error? How would DAPM deal with this case?

> +
> +	if (entity->num_sources != hweight64(control->cn_list)) {
> +		dev_err(dev, "%s: mismatched control and sources\n", entity->label);
> +		return -EINVAL;
> +	}
> +
> +	kctl = devm_kcalloc(dev, entity->num_sources, sizeof(*kctl), GFP_KERNEL);
> +	if (!kctl)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	for_each_set_bit(cn, (unsigned long *)&control->cn_list,
> +			 BITS_PER_TYPE(control->cn_list)) {
> +		const char *control_name;
> +		struct soc_mixer_control *mc;
> +
> +		control_name = devm_kasprintf(dev, GFP_KERNEL, "%s %d",
> +					      control->label, i + 1);
> +		if (!control_name)
> +			return -ENOMEM;
> +
> +		mc = devm_kmalloc(dev, sizeof(*mc), GFP_KERNEL);
> +		if (!mc)
> +			return -ENOMEM;
> +
> +		mc->reg = SND_SOC_NOPM;
> +		mc->rreg = SND_SOC_NOPM;
> +		mc->invert = 1; // Ensure default is connected
> +		mc->min = 0;
> +		mc->max = 1;
> +
> +		kctl[i].name = control_name;
> +		kctl[i].private_value = (unsigned long)mc;
> +		kctl[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +		kctl[i].info = snd_soc_info_volsw;
> +		kctl[i].get = snd_soc_dapm_get_volsw;
> +		kctl[i].put = snd_soc_dapm_put_volsw;
> +		i++;
> +	}
> +
> +	(*widget)->id = snd_soc_dapm_mixer;
> +	(*widget)->kcontrol_news = kctl;
> +	(*widget)->num_kcontrols = entity->num_sources;
> +	(*widget)++;
> +
> +	for (i = 0; i < entity->num_sources; i++)
> +		add_route(route, entity->label, kctl[i].name, entity->sources[i]->label);
> +
> +	return 0;
> +}

> +/**
> + * sdca_asoc_populate_dapm - fill in arrays of DAPM widgets and routes
> + * @dev: Pointer to the device against which allocations will be done.
> + * @function: Pointer to the Function information.
> + * @widget: Array of DAPM widgets to be populated.
> + * @route: Array of DAPM routes to be populated.
> + *
> + * This function populates arrays of DAPM widgets and routes from the
> + * DisCo information for a particular SDCA Function. Typically,
> + * snd_soc_asoc_count_component will be used to allocate appropriately
> + * sized arrays before calling this function.
> + *
> + * Return: Returns zero on success, and a negative error code on failure.
> + */
> +int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
> +			    struct snd_soc_dapm_widget *widget,
> +			    struct snd_soc_dapm_route *route)
> +{
> +	int ret;
> +	int i;
> +
> +	/* Some entities need to add controls referenced by other entities */
> +	for (i = 0; i < function->num_entities - 1; i++) {
> +		struct sdca_entity *entity = &function->entities[i];
> +
> +		switch (entity->type) {
> +		case SDCA_ENTITY_TYPE_GE:
> +			ret = entity_early_parse_ge(dev, function, entity);

what does 'early' mean here? is there a 'late_parse_ge', or is this to say that GEs are parsed first?

> +			if (ret)
> +				return ret;
> +			break;
> +		default:
> +			break;
> +		}
> +	}

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

On Mon, May 12, 2025 at 03:46:40PM +0200, Pierre-Louis Bossart wrote:
> On 5/12/25 14:42, Charles Keepax wrote:
> > SDCA also has a slight habit of having fully connected paths, relying
> > more on activating the PDEs to enable functionality. This doesn't
> > map quite so perfectly to DAPM which considers the path a reason to
> > power the PDE. Whilst in the current specification Mixer Units are
> > defined as fixed-function, in DAPM we create a virtual control for
> > each input (which defaults to connected). This allows paths to be
> > connected/disconnected, providing a more ASoC style approach to
> > managing the power. PIN_SWITCHs will also be added for non-dataport
> > terminal entities in a later patch along with the other ALSA controls,
> > providing greater flexibility in power management.
> 
> It's been a while since I reviewed an earlier versions so now
> I am confused.
> - Is the proposal to have a control to change the PDE state
> directly, as well as a virtual control to describe a path
> activation? That would lead to more flexibility but also to
> confusion, e.g. with 'no sound' when the paths are active but
> the PDE still in PS3.

No the PDEs are their own DAPM widgets and are controlled by
DAPM, they power up when they are on an active path. User-space
has no direct control over the power state.

The two methods user-space has for affecting the power state of
the system. Firstly, the virtual switches on the mixer units,
I initially added. These let you connect/disconnect a route into
the MU. Secondly, the pin switched on the endpoints that you
requested, these let you enable/disable a non-dataport endpoint.

> - Or do those virtual controls manage the PDE state, without
> userspace interaction? In that case the 'connected' default
> would higher power consumption until userspace decides which
> paths it really wants active.

All the pin switches default to on, but that is normal and
probably makes the most sense since that way paths will generally
work without any interaction.

> > +		switch (entity->type) {
> > +		case SDCA_ENTITY_TYPE_IT:
> > +		case SDCA_ENTITY_TYPE_OT:
> > +			*num_routes += !!entity->iot.clock;
> > +			*num_routes += !!entity->iot.is_dataport;
> 
> I couldn't follow those two lines.
> Why does the clock play a role, for now it's not really managed?

Clocks are managed, it is simply the clock muxes that are not
fully supported at the moment. Which is because clocks are supply
widgets and DAPM doesn't really have a concept of selectively
connecting supply widgets at the moment. Which is a little fiddly
to implement and given our parts don't use any clock muxes not
high on my priority list.

> And the difference between streaming and transducer terminals
> shouldn't change the accounting of routes?

Yes it does, remember everything is being mapped to DAPM widgets
here, so streaming terminals need a DAPM route in from the DAI
widget.

> > +			break;
> > +		case SDCA_ENTITY_TYPE_PDE:
> > +			*num_routes += entity->pde.num_managed;
> 
> same here, isn't there a risk of the same route being counted
> multiple times?

The PDE is implemented as a supply widget and it will be
connected to every widget it controls with a DAPM route to
signify that connection, this adds space for each of those
routes.

> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		*num_routes += entity->num_sources;
> > +
> > +		if (entity->group)
> > +			(*num_routes)++;
> 
> And here as well, in case an entity is controlled by a GE does
> the number of routes really change? 

Yes because the GE is also implemented as a supply widget and
linked to all the widgets it controls.

> I guess the meaning of 'routes' isn't really clear in my head,
> it's not just data paths but control as well that's taken into
> account?

I mean the hard definition here is DAPM routes. But roughly
speaking each line on the SDCA function diagram will correspond
to a route here, with a few tweaks like the GEs only show a
single line going to a red box around a bunch of other entities
that will be multiple routes one for each entity in the box.

> > +	texts[0] = "No Jack";
> > +	texts[1] = "Jack Unknown";
> > +	texts[2] = "Detection in Progress";
> > +	values[0] = 0;
> > +	values[1] = 1;
> > +	values[2] = 2;
> > +	for (i = 0; i < range->rows; i++) {
> > +		enum sdca_terminal_type type;
> > +
> > +		type = sdca_range(range, SDCA_SELECTED_MODE_TERM_TYPE, i);
> > +
> > +		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);
> 
> Humm, my memory of GEs is that the hardware reports a 'DETECTED_MODE' and
> then software (kernel and/or userspace) can choose a "SELECTED_MODE". here
> we only deal with SELECTED_MODE, is this intentional?
> 

This code is creating the ALSA control for the SELECTED_MODE
control, the DETECTED_MODE is all handled internally and there is
no need to export it.

> > +	if (entity->iot.is_dataport) {
> > +		const char *aif_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
> > +						      entity->label, "Playback");
> > +		if (!aif_name)
> > +			return -ENOMEM;
> > +
> > +		(*widget)->id = snd_soc_dapm_aif_in;
> > +
> > +		add_route(route, entity->label, NULL, aif_name);
> > +	} else {
> > +		(*widget)->id = snd_soc_dapm_mic;
> 
> so for non-dataport terminals there's no route added, but didn't the intro
> say that there was a virtual control for such terminals? that seems like a
> route definition if a control can alter the data path, no?
> 

This add route is for connecting the DAI widget, the actual
routes specified by the SDCA topology are added in the for loop
below.

And the PIN_SWITCH's are added in the next patch which adds the
non-DAPM ALSA controls, as noted in the commit message.

> > +	}
> > +
> > +	if (entity->iot.clock)
> > +		add_route(route, entity->label, NULL, entity->iot.clock->label);
> > +
> > +	for (i = 0; i < entity->num_sources; i++)
> > +		add_route(route, entity->label, NULL, entity->sources[i]->label);
> > +
> > +	(*widget)++;
> > +
> > +	return 0;
> > +}
> 
> > +static int entity_pde_event(struct snd_soc_dapm_widget *widget,
> > +			    struct snd_kcontrol *kctl, int event)
> > +{
> > +	struct snd_soc_component *component = widget->dapm->component;
> > +	struct sdca_entity *entity = widget->priv;
> > +	static const int poll_us = 10000;
> 
> This should be a #define, no?
> 

I could make it a define if you feel strong but I generally quite
like if it just a local delay being using in one place to define
it locally, means you don't have to look things up when reading
the code.

> > +	for (i = 0; i < polls; i++) {
> > +		if (i)
> > +			fsleep(poll_us);
> 
> The logic seems inverted? I would first sleep by the amount of time
> required for a transition before reading the status register. Here
> we read it first and then sleep for following iterations. The first
> read is almost guaranteed to fail.
> 

I mean it would be fine to sleep first but often there is enough
slack in the system that things are ready as soon as you get
here. Often these transitions are way way way faster than the max
time we are parsing from DisCo implies.

> > +static int entity_parse_pde(struct device *dev,
> > +			    struct sdca_function_data *function,
> > +			    struct sdca_entity *entity,
> > +			    struct snd_soc_dapm_widget **widget,
> > +			    struct snd_soc_dapm_route **route)
> > +{
> > +	unsigned int target = (1 << SDCA_PDE_PS0) | (1 << SDCA_PDE_PS3);
> > +	struct sdca_control_range *range;
> > +	struct sdca_control *control;
> > +	unsigned int mask = 0;
> > +	int i;
> > +
> > +	control = selector_find_control(dev, entity, SDCA_CTL_PDE_REQUESTED_PS);
> > +	if (!control)
> > +		return -EINVAL;
> > +
> > +	/* Power should only be controlled by the driver */
> 
> is it though? With the virtual controls you were referring to earlier, it's
> possible that the path is not used and the PDE in PS3. I guess I am still in
> the dark on what controls the PDE.

The PDE is controlled by the DAPM widget created at the bottom of
this function and yes power should only be directly controlled by
drivers.

> > +	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> > +	kctl->name = "Route";
> > +	kctl->info = snd_soc_info_enum_double;
> > +	kctl->get = snd_soc_dapm_get_enum_double;
> > +	kctl->put = snd_soc_dapm_put_enum_double;
> > +	kctl->private_value = (unsigned long)soc_enum;
> 
> Can you remind me in which cases we have a Selector Unit not
> controlled by a GE, and what userspace would do with such kcontrols?
> 

A selector unit not tidied to a GE is just a DAPM mux, it
provides an ALSA control which allows user-space to select which
input it wants routed through the selector.

> > +	/* MU control should be through DAPM */
> > +	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
> > +		dev_warn(dev, "%s: unexpected access layer: %x\n",
> > +			 entity->label, control->layers);
> 
> isn't this an error? How would DAPM deal with this case?

In general the control will work the same no matter what access
specifier it is given, it still sets the control. So I didn't
really see the need to hard error out here. Mostly likely if one
gets these its a sign of slightly dodgy DisCo but we should be
fine to carry one.

> > +int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
> > +			    struct snd_soc_dapm_widget *widget,
> > +			    struct snd_soc_dapm_route *route)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	/* Some entities need to add controls referenced by other entities */
> > +	for (i = 0; i < function->num_entities - 1; i++) {
> > +		struct sdca_entity *entity = &function->entities[i];
> > +
> > +		switch (entity->type) {
> > +		case SDCA_ENTITY_TYPE_GE:
> > +			ret = entity_early_parse_ge(dev, function, entity);
> 
> what does 'early' mean here? is there a 'late_parse_ge', or is this
> to say that GEs are parsed first?

Yeah we need to parse them first such that the GE controls
are ready when the SUs are parsed since they will link to the
control generated by the GE.

Thanks,
Charles

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Pierre-Louis Bossart]

Thanks for the comments Charles, I am fine with most of your points except the DETECTED_MODE and a question on the non-GE-managed-SUs, see below
-Pierre

>>> SDCA also has a slight habit of having fully connected paths, relying
>>> more on activating the PDEs to enable functionality. This doesn't
>>> map quite so perfectly to DAPM which considers the path a reason to
>>> power the PDE. Whilst in the current specification Mixer Units are
>>> defined as fixed-function, in DAPM we create a virtual control for
>>> each input (which defaults to connected). This allows paths to be
>>> connected/disconnected, providing a more ASoC style approach to
>>> managing the power. PIN_SWITCHs will also be added for non-dataport
>>> terminal entities in a later patch along with the other ALSA controls,
>>> providing greater flexibility in power management.
>>
>> It's been a while since I reviewed an earlier versions so now
>> I am confused.
>> - Is the proposal to have a control to change the PDE state
>> directly, as well as a virtual control to describe a path
>> activation? That would lead to more flexibility but also to
>> confusion, e.g. with 'no sound' when the paths are active but
>> the PDE still in PS3.
> 
> No the PDEs are their own DAPM widgets and are controlled by
> DAPM, they power up when they are on an active path. User-space
> has no direct control over the power state.
> 
> The two methods user-space has for affecting the power state of
> the system. Firstly, the virtual switches on the mixer units,
> I initially added. These let you connect/disconnect a route into
> the MU. Secondly, the pin switched on the endpoints that you
> requested, these let you enable/disable a non-dataport endpoint.

ok

>> - Or do those virtual controls manage the PDE state, without
>> userspace interaction? In that case the 'connected' default
>> would higher power consumption until userspace decides which
>> paths it really wants active.
> 
> All the pin switches default to on, but that is normal and
> probably makes the most sense since that way paths will generally
> work without any interaction.

ok

>>> +		switch (entity->type) {
>>> +		case SDCA_ENTITY_TYPE_IT:
>>> +		case SDCA_ENTITY_TYPE_OT:
>>> +			*num_routes += !!entity->iot.clock;
>>> +			*num_routes += !!entity->iot.is_dataport;
>>
>> I couldn't follow those two lines.
>> Why does the clock play a role, for now it's not really managed?
> 
> Clocks are managed, it is simply the clock muxes that are not
> fully supported at the moment. Which is because clocks are supply
> widgets and DAPM doesn't really have a concept of selectively
> connecting supply widgets at the moment. Which is a little fiddly
> to implement and given our parts don't use any clock muxes not
> high on my priority list.

ok, makes sense.

>> And the difference between streaming and transducer terminals
>> shouldn't change the accounting of routes?
> 
> Yes it does, remember everything is being mapped to DAPM widgets
> here, so streaming terminals need a DAPM route in from the DAI
> widget.

ah yes, you added one layer. I naively thought the DAI was the input/output terminal directly.
 
>>> +			break;
>>> +		case SDCA_ENTITY_TYPE_PDE:
>>> +			*num_routes += entity->pde.num_managed;
>>
>> same here, isn't there a risk of the same route being counted
>> multiple times?
> 
> The PDE is implemented as a supply widget and it will be
> connected to every widget it controls with a DAPM route to
> signify that connection, this adds space for each of those
> routes.

ok.

>>> +			break;
>>> +		default:
>>> +			break;
>>> +		}
>>> +
>>> +		*num_routes += entity->num_sources;
>>> +
>>> +		if (entity->group)
>>> +			(*num_routes)++;
>>
>> And here as well, in case an entity is controlled by a GE does
>> the number of routes really change? 
> 
> Yes because the GE is also implemented as a supply widget and
> linked to all the widgets it controls.
> 
>> I guess the meaning of 'routes' isn't really clear in my head,
>> it's not just data paths but control as well that's taken into
>> account?
> 
> I mean the hard definition here is DAPM routes. But roughly
> speaking each line on the SDCA function diagram will correspond
> to a route here, with a few tweaks like the GEs only show a
> single line going to a red box around a bunch of other entities
> that will be multiple routes one for each entity in the box.

This could be described in comments. The SDCA spec shows lines of different color for pure data paths and 'control' links. Here we are bringing everything back to the same concept of DAPM routes, but those extra routes only connect to supply widgets.

>>> +	texts[0] = "No Jack";
>>> +	texts[1] = "Jack Unknown";
>>> +	texts[2] = "Detection in Progress";
>>> +	values[0] = 0;
>>> +	values[1] = 1;
>>> +	values[2] = 2;
>>> +	for (i = 0; i < range->rows; i++) {
>>> +		enum sdca_terminal_type type;
>>> +
>>> +		type = sdca_range(range, SDCA_SELECTED_MODE_TERM_TYPE, i);
>>> +
>>> +		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);
>>
>> Humm, my memory of GEs is that the hardware reports a 'DETECTED_MODE' and
>> then software (kernel and/or userspace) can choose a "SELECTED_MODE". here
>> we only deal with SELECTED_MODE, is this intentional?
>>
> 
> This code is creating the ALSA control for the SELECTED_MODE
> control, the DETECTED_MODE is all handled internally and there is
> no need to export it.

Humm, that one doesn't seem right. There could be cases where the DETECTED_MODE is inconclusive, or that additional vendor-specific steps are needed to decide which SELECTED_MODE makes sense. I don't think we should assume everything is handled internally, or we have to define what 'internally' means. This cannot be generic SDCA stuff, it has to be configurable for specific implementations/vendors.

>>> +	if (entity->iot.is_dataport) {
>>> +		const char *aif_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
>>> +						      entity->label, "Playback");
>>> +		if (!aif_name)
>>> +			return -ENOMEM;
>>> +
>>> +		(*widget)->id = snd_soc_dapm_aif_in;
>>> +
>>> +		add_route(route, entity->label, NULL, aif_name);
>>> +	} else {
>>> +		(*widget)->id = snd_soc_dapm_mic;
>>
>> so for non-dataport terminals there's no route added, but didn't the intro
>> say that there was a virtual control for such terminals? that seems like a
>> route definition if a control can alter the data path, no?
>>
> 
> This add route is for connecting the DAI widget, the actual
> routes specified by the SDCA topology are added in the for loop
> below.
> 
> And the PIN_SWITCH's are added in the next patch which adds the
> non-DAPM ALSA controls, as noted in the commit message.

ok

>>> +	}
>>> +
>>> +	if (entity->iot.clock)
>>> +		add_route(route, entity->label, NULL, entity->iot.clock->label);
>>> +
>>> +	for (i = 0; i < entity->num_sources; i++)
>>> +		add_route(route, entity->label, NULL, entity->sources[i]->label);
>>> +
>>> +	(*widget)++;
>>> +
>>> +	return 0;
>>> +}
>>
>>> +static int entity_pde_event(struct snd_soc_dapm_widget *widget,
>>> +			    struct snd_kcontrol *kctl, int event)
>>> +{
>>> +	struct snd_soc_component *component = widget->dapm->component;
>>> +	struct sdca_entity *entity = widget->priv;
>>> +	static const int poll_us = 10000;
>>
>> This should be a #define, no?
>>
> 
> I could make it a define if you feel strong but I generally quite
> like if it just a local delay being using in one place to define
> it locally, means you don't have to look things up when reading
> the code.

I guess my main point was to explain where this 10ms value comes from. I am not sure what to make of it, it's either very large or very small depending on the initial and desired power states. Plus I would expect the sleep times to vary by orders of magnitude depending on the type of function managed.

>>> +	for (i = 0; i < polls; i++) {
>>> +		if (i)
>>> +			fsleep(poll_us);
>>
>> The logic seems inverted? I would first sleep by the amount of time
>> required for a transition before reading the status register. Here
>> we read it first and then sleep for following iterations. The first
>> read is almost guaranteed to fail.
>>
> 
> I mean it would be fine to sleep first but often there is enough
> slack in the system that things are ready as soon as you get
> here. Often these transitions are way way way faster than the max
> time we are parsing from DisCo implies.

Sorry, my bad. I thought the DisCo definitions were minimum sleep times, as for HDaudio. But they are worst-case times, so it makes sense to try one's luck early and wait later.

>>> +static int entity_parse_pde(struct device *dev,
>>> +			    struct sdca_function_data *function,
>>> +			    struct sdca_entity *entity,
>>> +			    struct snd_soc_dapm_widget **widget,
>>> +			    struct snd_soc_dapm_route **route)
>>> +{
>>> +	unsigned int target = (1 << SDCA_PDE_PS0) | (1 << SDCA_PDE_PS3);
>>> +	struct sdca_control_range *range;
>>> +	struct sdca_control *control;
>>> +	unsigned int mask = 0;
>>> +	int i;
>>> +
>>> +	control = selector_find_control(dev, entity, SDCA_CTL_PDE_REQUESTED_PS);
>>> +	if (!control)
>>> +		return -EINVAL;
>>> +
>>> +	/* Power should only be controlled by the driver */
>>
>> is it though? With the virtual controls you were referring to earlier, it's
>> possible that the path is not used and the PDE in PS3. I guess I am still in
>> the dark on what controls the PDE.
> 
> The PDE is controlled by the DAPM widget created at the bottom of
> this function and yes power should only be directly controlled by
> drivers.

ok

>>> +	kctl->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>>> +	kctl->name = "Route";
>>> +	kctl->info = snd_soc_info_enum_double;
>>> +	kctl->get = snd_soc_dapm_get_enum_double;
>>> +	kctl->put = snd_soc_dapm_put_enum_double;
>>> +	kctl->private_value = (unsigned long)soc_enum;
>>
>> Can you remind me in which cases we have a Selector Unit not
>> controlled by a GE, and what userspace would do with such kcontrols?
>>
> 
> A selector unit not tidied to a GE is just a DAPM mux, it
> provides an ALSA control which allows user-space to select which
> input it wants routed through the selector.

I meant in which topologies is this used/needed?

>>> +	/* MU control should be through DAPM */
>>> +	if (control->layers != SDCA_ACCESS_LAYER_CLASS)
>>> +		dev_warn(dev, "%s: unexpected access layer: %x\n",
>>> +			 entity->label, control->layers);
>>
>> isn't this an error? How would DAPM deal with this case?
> 
> In general the control will work the same no matter what access
> specifier it is given, it still sets the control. So I didn't
> really see the need to hard error out here. Mostly likely if one
> gets these its a sign of slightly dodgy DisCo but we should be
> fine to carry one.

ok, probably best to deal with flaky DSDT at a later time.

>>> +int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
>>> +			    struct snd_soc_dapm_widget *widget,
>>> +			    struct snd_soc_dapm_route *route)
>>> +{
>>> +	int ret;
>>> +	int i;
>>> +
>>> +	/* Some entities need to add controls referenced by other entities */
>>> +	for (i = 0; i < function->num_entities - 1; i++) {
>>> +		struct sdca_entity *entity = &function->entities[i];
>>> +
>>> +		switch (entity->type) {
>>> +		case SDCA_ENTITY_TYPE_GE:
>>> +			ret = entity_early_parse_ge(dev, function, entity);
>>
>> what does 'early' mean here? is there a 'late_parse_ge', or is this
>> to say that GEs are parsed first?
> 
> Yeah we need to parse them first such that the GE controls
> are ready when the SUs are parsed since they will link to the
> control generated by the GE.

ok, worthy of a comment to make this dependency clearer.

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

On Wed, May 14, 2025 at 02:15:01PM +0200, Pierre-Louis Bossart wrote:
> > I mean the hard definition here is DAPM routes. But roughly
> > speaking each line on the SDCA function diagram will correspond
> > to a route here, with a few tweaks like the GEs only show a
> > single line going to a red box around a bunch of other entities
> > that will be multiple routes one for each entity in the box.
> 
> This could be described in comments. The SDCA spec shows lines
> of different color for pure data paths and 'control' links. Here
> we are bringing everything back to the same concept of DAPM
> routes, but those extra routes only connect to supply widgets.

Yeah I am happy to add extra comments for this.

> >>> +		values[i + 3] = sdca_range(range, SDCA_SELECTED_MODE_INDEX, i);
> >>
> >> Humm, my memory of GEs is that the hardware reports a 'DETECTED_MODE' and
> >> then software (kernel and/or userspace) can choose a "SELECTED_MODE". here
> >> we only deal with SELECTED_MODE, is this intentional?
> > 
> > This code is creating the ALSA control for the SELECTED_MODE
> > control, the DETECTED_MODE is all handled internally and there is
> > no need to export it.
> 
> Humm, that one doesn't seem right. There could be cases
> where the DETECTED_MODE is inconclusive, or that additional
> vendor-specific steps are needed to decide which SELECTED_MODE
> makes sense. I don't think we should assume everything is
> handled internally, or we have to define what 'internally'
> means. This cannot be generic SDCA stuff, it has to be
> configurable for specific implementations/vendors.

Ok yeah I see, the current code reads the DETECTED_MODE in
the IRQ handler and then updates the selected mode. I guess
currently what is missing is the handling for the JACK_UNKNOWN
case, which could probably require user-space. I think it would
be pretty easy to export the DETECTED_MODE as well I will have a
look.

> >>> +static int entity_pde_event(struct snd_soc_dapm_widget *widget,
> >>> +			    struct snd_kcontrol *kctl, int event)
> >>> +{
> >>> +	struct snd_soc_component *component = widget->dapm->component;
> >>> +	struct sdca_entity *entity = widget->priv;
> >>> +	static const int poll_us = 10000;
> >>
> >> This should be a #define, no?
> >>
> > 
> > I could make it a define if you feel strong but I generally quite
> > like if it just a local delay being using in one place to define
> > it locally, means you don't have to look things up when reading
> > the code.
> 
> I guess my main point was to explain where this 10ms value
> comes from. I am not sure what to make of it, it's either very
> large or very small depending on the initial and desired power
> states. Plus I would expect the sleep times to vary by orders
> of magnitude depending on the type of function managed.

The trouble is there isn't a great way to parse from the DisCo
what a good poll interval would be. I guess we could change to
say 1/100th of the maximum, clipped to a minimum time? My concern
is the maximums can be quite long so something based on them
definitely runs the risk of really under polling and introducing
a lot of lag.

> >> Can you remind me in which cases we have a Selector Unit not
> >> controlled by a GE, and what userspace would do with such kcontrols?
> >>
> > 
> > A selector unit not tidied to a GE is just a DAPM mux, it
> > provides an ALSA control which allows user-space to select which
> > input it wants routed through the selector.
> 
> I meant in which topologies is this used/needed?

Its also nice to support topologies above the set ones in the
spec, but the current in spec users of these would be:

UAJ:
SU136 - Which lets you mux the second jack capture onto the primary
capture path.
SU137 - Which lets you sidetone from the second jack capture.

Admittedly our chips don't currently have those but since I
implemented SU support before GE support we had code for this
anyway.

> >>> +		case SDCA_ENTITY_TYPE_GE:
> >>> +			ret = entity_early_parse_ge(dev, function, entity);
> >>
> >> what does 'early' mean here? is there a 'late_parse_ge', or is this
> >> to say that GEs are parsed first?
> > 
> > Yeah we need to parse them first such that the GE controls
> > are ready when the SUs are parsed since they will link to the
> > control generated by the GE.
> 
> ok, worthy of a comment to make this dependency clearer.

Yeah no problem to add a comment for that.

Thanks,
Charles

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

On Wed, May 14, 2025 at 02:30:00PM +0100, Charles Keepax wrote:
> On Wed, May 14, 2025 at 02:15:01PM +0200, Pierre-Louis Bossart wrote:
> > Humm, that one doesn't seem right. There could be cases
> > where the DETECTED_MODE is inconclusive, or that additional
> > vendor-specific steps are needed to decide which SELECTED_MODE
> > makes sense. I don't think we should assume everything is
> > handled internally, or we have to define what 'internally'
> > means. This cannot be generic SDCA stuff, it has to be
> > configurable for specific implementations/vendors.
> 
> Ok yeah I see, the current code reads the DETECTED_MODE in
> the IRQ handler and then updates the selected mode. I guess
> currently what is missing is the handling for the JACK_UNKNOWN
> case, which could probably require user-space. I think it would
> be pretty easy to export the DETECTED_MODE as well I will have a
> look.

Should hopefully be able to send a new spin tomorrow. I have
managed to include all the comments I think.

I started being a little more unsure on this DETECTED_MODE again,
but I am going to go with forcing the export of DETECTED_MODE
for now and we can revise later if we have a better plan.

My concerns really are most custom handling will actually
want to live in the kernel, except the case where the system
wants to present a dialog to the user to select the jack device
manually. In that manual case I also can't see any reason why
just setting the selected mode to unknown would not make the
most sense, but the spec does not say the device should do that.

Thanks,
Charles

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Pierre-Louis Bossart]

On 5/15/25 16:50, Charles Keepax wrote:
> On Wed, May 14, 2025 at 02:30:00PM +0100, Charles Keepax wrote:
>> On Wed, May 14, 2025 at 02:15:01PM +0200, Pierre-Louis Bossart wrote:
>>> Humm, that one doesn't seem right. There could be cases
>>> where the DETECTED_MODE is inconclusive, or that additional
>>> vendor-specific steps are needed to decide which SELECTED_MODE
>>> makes sense. I don't think we should assume everything is
>>> handled internally, or we have to define what 'internally'
>>> means. This cannot be generic SDCA stuff, it has to be
>>> configurable for specific implementations/vendors.
>>
>> Ok yeah I see, the current code reads the DETECTED_MODE in
>> the IRQ handler and then updates the selected mode. I guess
>> currently what is missing is the handling for the JACK_UNKNOWN
>> case, which could probably require user-space. I think it would
>> be pretty easy to export the DETECTED_MODE as well I will have a
>> look.
> 
> Should hopefully be able to send a new spin tomorrow. I have
> managed to include all the comments I think.
> 
> I started being a little more unsure on this DETECTED_MODE again,
> but I am going to go with forcing the export of DETECTED_MODE
> for now and we can revise later if we have a better plan.
> 
> My concerns really are most custom handling will actually
> want to live in the kernel, except the case where the system
> wants to present a dialog to the user to select the jack device
> manually. In that manual case I also can't see any reason why
> just setting the selected mode to unknown would not make the
> most sense, but the spec does not say the device should do that.

I am fine with a TODO on the DETECTED/SELECTED mode handling for now. I do recall discussions in the Audio Subgroup for the Retaskable Jack where there are just too many options and a basic driver would probably give up and say "undefined" for the DETECTED_MODE. For the UAJ mirroring the DETECTED_MODE and/or asking the user for input are probably the two most likely options.

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

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

On Mon, May 12, 2025 at 03:46:40PM +0200, Pierre-Louis Bossart wrote:
> On 5/12/25 14:42, Charles Keepax wrote:
> It's been a while since I reviewed an earlier versions so now I am confused.

I have attached the output of the dapm-graph tool for the UAJ
component for one of our devices, whilst a headset is connected.
Hopefully this will help to make it a little more clear what
the generated graph from this code looks like. Hopefully the
attachment makes it through the ether, let me know if it doesn't
and I can send directly.

The PDE supply widgets are what control the PDE power registers,
they will power up when they are attached to an active path, ie.
a valid link between an active source and an active sink.

For the streaming terminals IT41,OT36 they are active when the
audio stream is open. For the non-streaming ones
IT31,IT32,IT33,OT43,OT44,OT45 these are active when the pin
switch is enabled which it is by default, without pin switches
these would be always active. The pin switches are not shown
on the output of dapm-graph.

You can see the effect of the GE control, where it has set the
input of SU35 to FU33, by the selection of "Headset". And you can
see the virtual mixer switches going into MU35, currently both
Mixer 1 and 2 are on, so you can see the paths connected.

But as you can see the correlation to the function diagrams is
the spec is pretty good. As I noted with a few minor exceptions,
like the GE having more paths, since it must connect to all the
widgets it controls separately. And the DAI widgets that are
created by the DAPM core.

Thanks,
Charles

[-- Attachment #2: test.png --]
[-- Type: image/png, Size: 101091 bytes --]

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Pierre-Louis Bossart]

On 5/13/25 11:56, Charles Keepax wrote:
> On Mon, May 12, 2025 at 03:46:40PM +0200, Pierre-Louis Bossart wrote:
>> On 5/12/25 14:42, Charles Keepax wrote:
>> It's been a while since I reviewed an earlier versions so now I am confused.
> 
> I have attached the output of the dapm-graph tool for the UAJ
> component for one of our devices, whilst a headset is connected.
> Hopefully this will help to make it a little more clear what
> the generated graph from this code looks like. Hopefully the
> attachment makes it through the ether, let me know if it doesn't
> and I can send directly.
> 
> The PDE supply widgets are what control the PDE power registers,
> they will power up when they are attached to an active path, ie.
> a valid link between an active source and an active sink.
> 
> For the streaming terminals IT41,OT36 they are active when the
> audio stream is open. For the non-streaming ones
> IT31,IT32,IT33,OT43,OT44,OT45 these are active when the pin
> switch is enabled which it is by default, without pin switches
> these would be always active. The pin switches are not shown
> on the output of dapm-graph.
> 
> You can see the effect of the GE control, where it has set the
> input of SU35 to FU33, by the selection of "Headset". And you can
> see the virtual mixer switches going into MU35, currently both
> Mixer 1 and 2 are on, so you can see the paths connected.
> 
> But as you can see the correlation to the function diagrams is
> the spec is pretty good. As I noted with a few minor exceptions,
> like the GE having more paths, since it must connect to all the
> widgets it controls separately. And the DAI widgets that are
> created by the DAPM core.

Thanks for the diagram, very useful indeed.

The only comment I'd have is that all SUs are represented as MUXes in this DAPM graph, but in practice most of them are simple on/off switches. The case of SU35 is one of the exceptions to the rule where it can have multiple inputs. I don't really mind if all SUs are represented as MUX, but it's not a direct mapping from the SDCA spec.

Re: [PATCH v5 4/6] ASoC: SDCA: Create DAPM widgets and routes from DisCo

[Charles Keepax]

On Wed, May 14, 2025 at 02:33:23PM +0200, Pierre-Louis Bossart wrote:
> On 5/13/25 11:56, Charles Keepax wrote:
> > On Mon, May 12, 2025 at 03:46:40PM +0200, Pierre-Louis Bossart wrote:
> >> On 5/12/25 14:42, Charles Keepax wrote:
> > But as you can see the correlation to the function diagrams is
> > the spec is pretty good. As I noted with a few minor exceptions,
> > like the GE having more paths, since it must connect to all the
> > widgets it controls separately. And the DAI widgets that are
> > created by the DAPM core.
> 
> Thanks for the diagram, very useful indeed.
> 
> The only comment I'd have is that all SUs are represented
> as MUXes in this DAPM graph, but in practice most of them
> are simple on/off switches. The case of SU35 is one of the
> exceptions to the rule where it can have multiple inputs. I
> don't really mind if all SUs are represented as MUX, but it's
> not a direct mapping from the SDCA spec.

I mean I think we could special case 1 input versions as
switches but a) I am slightly concerned it might make sharing
them across a GE harder and b) I am not sure I see what it
really buys us.  Generally I prefer the code to not require
special casing for certain situations and there shouldn't be
any difference in the behaviour from switches or muxes.

Thanks,
Charles

[PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Charles Keepax]

Use the previously parsed DisCo information from ACPI to create the
ALSA controls required by an SDCA Function. This maps all User and
Application level SDCA Controls to ALSA controls. Typically controls
marked with those access levels are just volumes and mutes.

SDCA defines volume controls as an integer in 1/256ths of a dB and
then provides a mechanism to specify what values are valid (range
templates). Currently only a simple case of a single linear volume
range with a power of 2 step size is supported. This allows the code
to expose the volume control using a simple shift. This will need
expanded in the future, to support more complex ranges and probably
also some additional control types but this should be sufficient to
for a first pass.

For non-dataport terminal widgets also add a pin switch to allow
that endpoint to be turned on/off.

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

No changes since v4.

 include/sound/sdca_asoc.h     |   6 +-
 include/sound/sdca_function.h |  10 ++
 sound/soc/sdca/sdca_asoc.c    | 241 +++++++++++++++++++++++++++++++++-
 3 files changed, 252 insertions(+), 5 deletions(-)

diff --git a/include/sound/sdca_asoc.h b/include/sound/sdca_asoc.h
index 414d461b6fc4a..d19e7e969283a 100644
--- a/include/sound/sdca_asoc.h
+++ b/include/sound/sdca_asoc.h
@@ -12,16 +12,20 @@
 
 struct device;
 struct sdca_function_data;
+struct snd_kcontrol_new;
 struct snd_soc_component_driver;
 struct snd_soc_dapm_route;
 struct snd_soc_dapm_widget;
 
 int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
-			      int *num_widgets, int *num_routes);
+			      int *num_widgets, int *num_routes, int *num_controls);
 
 int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
 			    struct snd_soc_dapm_widget *widgets,
 			    struct snd_soc_dapm_route *routes);
+int sdca_asoc_populate_controls(struct device *dev,
+				struct sdca_function_data *function,
+				struct snd_kcontrol_new *kctl);
 
 int sdca_asoc_populate_component(struct device *dev,
 				 struct sdca_function_data *function,
diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 83fedc39cf714..77ffb1f4e1ca9 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -206,6 +206,16 @@ enum sdca_fu_controls {
 	SDCA_CTL_FU_LATENCY				= 0x10,
 };
 
+/**
+ * enum sdca_volume_range - Column definitions for Q7.8dB volumes/gains
+ */
+enum sdca_volume_range {
+	SDCA_VOLUME_LINEAR_MIN				= 0,
+	SDCA_VOLUME_LINEAR_MAX				= 1,
+	SDCA_VOLUME_LINEAR_STEP				= 2,
+	SDCA_VOLUME_LINEAR_NCOLS			= 3,
+};
+
 /**
  * enum sdca_xu_controls - SDCA Controls for Extension Unit
  *
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index a0c1eb99e97b4..e3debfe55eb04 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -21,6 +21,7 @@
 #include <sound/soc.h>
 #include <sound/soc-component.h>
 #include <sound/soc-dapm.h>
+#include <sound/tlv.h>
 
 static struct sdca_control *selector_find_control(struct device *dev,
 						  struct sdca_entity *entity,
@@ -69,6 +70,16 @@ static struct sdca_control_range *selector_find_range(struct device *dev,
 	return control_find_range(dev, entity, control, cols, rows);
 }
 
+static bool exported_control(struct sdca_control *control)
+{
+	/* No need to export control for something that only has one value */
+	if (control->has_fixed)
+		return false;
+
+	return control->layers & (SDCA_ACCESS_LAYER_USER |
+				  SDCA_ACCESS_LAYER_APPLICATION);
+}
+
 /**
  * sdca_asoc_count_component - count the various component parts
  * @function: Pointer to the Function information.
@@ -76,6 +87,8 @@ static struct sdca_control_range *selector_find_range(struct device *dev,
  * required number of DAPM widgets for the Function.
  * @num_routes: Output integer pointer, will be filled with the
  * required number of DAPM routes for the Function.
+ * @num_controls: Output integer pointer, will be filled with the
+ * required number of ALSA controls for the Function.
  *
  * This function counts various things within the SDCA Function such
  * that the calling driver can allocate appropriate space before
@@ -84,12 +97,13 @@ static struct sdca_control_range *selector_find_range(struct device *dev,
  * Return: Returns zero on success, and a negative error code on failure.
  */
 int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
-			      int *num_widgets, int *num_routes)
+			      int *num_widgets, int *num_routes, int *num_controls)
 {
-	int i;
+	int i, j;
 
 	*num_widgets = function->num_entities - 1;
 	*num_routes = 0;
+	*num_controls = 0;
 
 	for (i = 0; i < function->num_entities - 1; i++) {
 		struct sdca_entity *entity = &function->entities[i];
@@ -99,6 +113,7 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
 		case SDCA_ENTITY_TYPE_OT:
 			*num_routes += !!entity->iot.clock;
 			*num_routes += !!entity->iot.is_dataport;
+			*num_controls += !entity->iot.is_dataport;
 			break;
 		case SDCA_ENTITY_TYPE_PDE:
 			*num_routes += entity->pde.num_managed;
@@ -111,6 +126,11 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
 
 		if (entity->group)
 			(*num_routes)++;
+
+		for (j = 0; j < entity->num_controls; j++) {
+			if (exported_control(&entity->controls[j]))
+				(*num_controls)++;
+		}
 	}
 
 	return 0;
@@ -792,6 +812,207 @@ int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *funct
 }
 EXPORT_SYMBOL_NS(sdca_asoc_populate_dapm, "SND_SOC_SDCA");
 
+static int control_limit_kctl(struct device *dev,
+			      struct sdca_entity *entity,
+			      struct sdca_control *control,
+			      struct snd_kcontrol_new *kctl)
+{
+	struct soc_mixer_control *mc = (struct soc_mixer_control *)kctl->private_value;
+	struct sdca_control_range *range;
+	int min, max, step;
+	unsigned int *tlv;
+	int shift;
+
+	if (control->type != SDCA_CTL_DATATYPE_Q7P8DB)
+		return 0;
+
+	/*
+	 * FIXME: For now only handle the simple case of a single linear range
+	 */
+	range = control_find_range(dev, entity, control, SDCA_VOLUME_LINEAR_NCOLS, 1);
+	if (!range)
+		return -EINVAL;
+
+	min = sdca_range(range, SDCA_VOLUME_LINEAR_MIN, 0);
+	max = sdca_range(range, SDCA_VOLUME_LINEAR_MAX, 0);
+	step = sdca_range(range, SDCA_VOLUME_LINEAR_STEP, 0);
+
+	min = sign_extend32(min, control->nbits - 1);
+	max = sign_extend32(max, control->nbits - 1);
+
+	/*
+	 * FIXME: Only support power of 2 step sizes as this can be supported
+	 * by a simple shift.
+	 */
+	if (hweight32(step) != 1) {
+		dev_err(dev, "%s: %s: currently unsupported step size\n",
+			entity->label, control->label);
+		return -EINVAL;
+	}
+
+	/*
+	 * The SDCA volumes are in steps of 1/256th of a dB, a step down of
+	 * 64 (shift of 6) gives 1/4dB. 1/4dB is the smallest unit that is also
+	 * representable in the ALSA TLVs which are in 1/100ths of a dB.
+	 */
+	shift = max(ffs(step) - 1, 6);
+
+	tlv = devm_kcalloc(dev, 4, sizeof(*tlv), GFP_KERNEL);
+	if (!tlv)
+		return -ENOMEM;
+
+	tlv[0] = SNDRV_CTL_TLVT_DB_SCALE;
+	tlv[1] = 2 * sizeof(*tlv);
+	tlv[2] = (min * 100) >> 8;
+	tlv[3] = ((1 << shift) * 100) >> 8;
+
+	mc->min = min >> shift;
+	mc->max = max >> shift;
+	mc->shift = shift;
+	mc->rshift = shift;
+	mc->sign_bit = 15 - shift;
+
+	kctl->access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_READWRITE;
+	kctl->tlv.p = tlv;
+
+	return 0;
+}
+
+static int populate_control(struct device *dev,
+			    struct sdca_function_data *function,
+			    struct sdca_entity *entity,
+			    struct sdca_control *control,
+			    struct snd_kcontrol_new **kctl)
+{
+	const char *control_suffix = "";
+	const char *control_name;
+	struct soc_mixer_control *mc;
+	int index = 0;
+	int ret;
+	int cn;
+
+	if (!exported_control(control))
+		return 0;
+
+	if (control->type == SDCA_CTL_DATATYPE_ONEBIT)
+		control_suffix = " Switch";
+
+	control_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s%s", entity->label,
+				      control->label, control_suffix);
+	if (!control_name)
+		return -ENOMEM;
+
+	mc = devm_kmalloc(dev, sizeof(*mc), GFP_KERNEL);
+	if (!mc)
+		return -ENOMEM;
+
+	for_each_set_bit(cn, (unsigned long *)&control->cn_list,
+			 BITS_PER_TYPE(control->cn_list)) {
+		switch (index++) {
+		case 0:
+			mc->reg = SDW_SDCA_CTL(function->desc->adr, entity->id,
+					       control->sel, cn);
+			mc->rreg = mc->reg;
+			break;
+		case 1:
+			mc->rreg = SDW_SDCA_CTL(function->desc->adr, entity->id,
+						control->sel, cn);
+			break;
+		default:
+			dev_err(dev, "%s: %s: only mono/stereo controls supported\n",
+				entity->label, control->label);
+			return -EINVAL;
+		}
+	}
+
+	mc->min = 0;
+	mc->max = (0x1ull << control->nbits) - 1;
+
+	(*kctl)->name = control_name;
+	(*kctl)->private_value = (unsigned long)mc;
+	(*kctl)->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	(*kctl)->info = snd_soc_info_volsw;
+	(*kctl)->get = snd_soc_get_volsw;
+	(*kctl)->put = snd_soc_put_volsw;
+
+	ret = control_limit_kctl(dev, entity, control, *kctl);
+	if (ret)
+		return ret;
+
+	(*kctl)++;
+
+	return 0;
+}
+
+static int populate_pin_switch(struct device *dev,
+			       struct sdca_entity *entity,
+			       struct snd_kcontrol_new **kctl)
+{
+	const char *control_name;
+
+	control_name = devm_kasprintf(dev, GFP_KERNEL, "%s Switch", entity->label);
+	if (!control_name)
+		return -ENOMEM;
+
+	(*kctl)->name = control_name;
+	(*kctl)->private_value = (unsigned long)entity->label;
+	(*kctl)->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	(*kctl)->info = snd_soc_dapm_info_pin_switch;
+	(*kctl)->get = snd_soc_dapm_get_component_pin_switch;
+	(*kctl)->put = snd_soc_dapm_put_component_pin_switch;
+	(*kctl)++;
+
+	return 0;
+}
+
+/**
+ * sdca_asoc_populate_controls - fill in an array of ALSA controls for a Function
+ * @dev: Pointer to the device against which allocations will be done.
+ * @function: Pointer to the Function information.
+ * @route: Array of ALSA controls to be populated.
+ *
+ * This function populates an array of ALSA controls from the DisCo
+ * information for a particular SDCA Function. Typically,
+ * snd_soc_asoc_count_component will be used to allocate an
+ * appropriately sized array before calling this function.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+int sdca_asoc_populate_controls(struct device *dev,
+				struct sdca_function_data *function,
+				struct snd_kcontrol_new *kctl)
+{
+	int i, j;
+	int ret;
+
+	for (i = 0; i < function->num_entities; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		switch (entity->type) {
+		case SDCA_ENTITY_TYPE_IT:
+		case SDCA_ENTITY_TYPE_OT:
+			if (!entity->iot.is_dataport) {
+				ret = populate_pin_switch(dev, entity, &kctl);
+				if (ret)
+					return ret;
+			}
+			break;
+		default:
+			break;
+		}
+
+		for (j = 0; j < entity->num_controls; j++) {
+			ret = populate_control(dev, function, entity,
+					       &entity->controls[j], &kctl);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_asoc_populate_controls, "SND_SOC_SDCA");
+
 /**
  * sdca_asoc_populate_component - fill in a component driver for a Function
  * @dev: Pointer to the device against which allocations will be done.
@@ -810,10 +1031,12 @@ int sdca_asoc_populate_component(struct device *dev,
 {
 	struct snd_soc_dapm_widget *widgets;
 	struct snd_soc_dapm_route *routes;
-	int num_widgets, num_routes;
+	struct snd_kcontrol_new *controls;
+	int num_widgets, num_routes, num_controls;
 	int ret;
 
-	ret = sdca_asoc_count_component(dev, function, &num_widgets, &num_routes);
+	ret = sdca_asoc_count_component(dev, function, &num_widgets, &num_routes,
+					&num_controls);
 	if (ret)
 		return ret;
 
@@ -825,14 +1048,24 @@ int sdca_asoc_populate_component(struct device *dev,
 	if (!routes)
 		return -ENOMEM;
 
+	controls = devm_kcalloc(dev, num_controls, sizeof(*controls), GFP_KERNEL);
+	if (!controls)
+		return -ENOMEM;
+
 	ret = sdca_asoc_populate_dapm(dev, function, widgets, routes);
 	if (ret)
 		return ret;
 
+	ret = sdca_asoc_populate_controls(dev, function, controls);
+	if (ret)
+		return ret;
+
 	component_drv->dapm_widgets = widgets;
 	component_drv->num_dapm_widgets = num_widgets;
 	component_drv->dapm_routes = routes;
 	component_drv->num_dapm_routes = num_routes;
+	component_drv->controls = controls;
+	component_drv->num_controls = num_controls;
 
 	return 0;
 }
-- 
2.39.5

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Pierre-Louis Bossart]

> +static bool exported_control(struct sdca_control *control)
> +{
> +	/* No need to export control for something that only has one value */
> +	if (control->has_fixed)
> +		return false;

why is that? It'd simplify userspace if there was always a way to get a value, even if it's fixed?

> +
> +	return control->layers & (SDCA_ACCESS_LAYER_USER |
> +				  SDCA_ACCESS_LAYER_APPLICATION);

That filter on access layer does make sense however.

> +}
> +
>  /**
>   * sdca_asoc_count_component - count the various component parts
>   * @function: Pointer to the Function information.
> @@ -76,6 +87,8 @@ static struct sdca_control_range *selector_find_range(struct device *dev,
>   * required number of DAPM widgets for the Function.
>   * @num_routes: Output integer pointer, will be filled with the
>   * required number of DAPM routes for the Function.
> + * @num_controls: Output integer pointer, will be filled with the
> + * required number of ALSA controls for the Function.
>   *
>   * This function counts various things within the SDCA Function such
>   * that the calling driver can allocate appropriate space before
> @@ -84,12 +97,13 @@ static struct sdca_control_range *selector_find_range(struct device *dev,
>   * Return: Returns zero on success, and a negative error code on failure.
>   */
>  int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
> -			      int *num_widgets, int *num_routes)
> +			      int *num_widgets, int *num_routes, int *num_controls)
>  {
> -	int i;
> +	int i, j;
>  
>  	*num_widgets = function->num_entities - 1;
>  	*num_routes = 0;
> +	*num_controls = 0;
>  
>  	for (i = 0; i < function->num_entities - 1; i++) {
>  		struct sdca_entity *entity = &function->entities[i];
> @@ -99,6 +113,7 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
>  		case SDCA_ENTITY_TYPE_OT:
>  			*num_routes += !!entity->iot.clock;
>  			*num_routes += !!entity->iot.is_dataport;
> +			*num_controls += !entity->iot.is_dataport;

shouldn't the same line be added for TYPE_IT?

>  			break;
>  		case SDCA_ENTITY_TYPE_PDE:
>  			*num_routes += entity->pde.num_managed;
> @@ -111,6 +126,11 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
>  
>  		if (entity->group)
>  			(*num_routes)++;
> +
> +		for (j = 0; j < entity->num_controls; j++) {
> +			if (exported_control(&entity->controls[j]))
> +				(*num_controls)++;
> +		}
>  	}
>  
>  	return 0;

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Charles Keepax]

On Mon, May 12, 2025 at 03:53:36PM +0200, Pierre-Louis Bossart wrote:
> 
> > +static bool exported_control(struct sdca_control *control)
> > +{
> > +	/* No need to export control for something that only has one value */
> > +	if (control->has_fixed)
> > +		return false;
> 
> why is that? It'd simplify userspace if there was always a way to
> get a value, even if it's fixed?

Well mostly it is because various things in the stack get
sad with a single value control you can't set. If we wanted
to change that I would suggest we add a FIXME. It isn't really
critical and I would really like to get more of this class
driver stuff moving, it has been out for review for a really long
time now.

> > @@ -99,6 +113,7 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
> >  		case SDCA_ENTITY_TYPE_OT:
> >  			*num_routes += !!entity->iot.clock;
> >  			*num_routes += !!entity->iot.is_dataport;
> > +			*num_controls += !entity->iot.is_dataport;
> 
> shouldn't the same line be added for TYPE_IT?

This is just the cutting on the patch generation, case
SDCA_ENTITY_TYPE_IT: is the line right above so this runs for
both.

Thanks,
Charles

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Charles Keepax]

On Mon, May 12, 2025 at 06:14:00PM +0100, Charles Keepax wrote:
> On Mon, May 12, 2025 at 03:53:36PM +0200, Pierre-Louis Bossart wrote:
> > 
> > > +static bool exported_control(struct sdca_control *control)
> > > +{
> > > +	/* No need to export control for something that only has one value */
> > > +	if (control->has_fixed)
> > > +		return false;
> > 
> > why is that? It'd simplify userspace if there was always a way to
> > get a value, even if it's fixed?
> 
> Well mostly it is because various things in the stack get
> sad with a single value control you can't set. If we wanted
> to change that I would suggest we add a FIXME. It isn't really
> critical and I would really like to get more of this class
> driver stuff moving, it has been out for review for a really long
> time now.
> 

Thinking a little more about this user-space is going to have to
handle controls not being there as different controls will be
exported across different parts anyway, based of which options
are implemented. Also we have seen a bit of people hiding
controls by changing the access layer. Although sometimes that
does result in things that are technically not class conformant
but they do work through the Microsoft drivers, so we will likely
need to support it anyway.

Thanks,
Charles

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Pierre-Louis Bossart]

On 5/13/25 12:24, Charles Keepax wrote:
> On Mon, May 12, 2025 at 06:14:00PM +0100, Charles Keepax wrote:
>> On Mon, May 12, 2025 at 03:53:36PM +0200, Pierre-Louis Bossart wrote:
>>>
>>>> +static bool exported_control(struct sdca_control *control)
>>>> +{
>>>> +	/* No need to export control for something that only has one value */
>>>> +	if (control->has_fixed)
>>>> +		return false;
>>>
>>> why is that? It'd simplify userspace if there was always a way to
>>> get a value, even if it's fixed?
>>
>> Well mostly it is because various things in the stack get
>> sad with a single value control you can't set. If we wanted
>> to change that I would suggest we add a FIXME. It isn't really
>> critical and I would really like to get more of this class
>> driver stuff moving, it has been out for review for a really long
>> time now.
>>
> 
> Thinking a little more about this user-space is going to have to
> handle controls not being there as different controls will be
> exported across different parts anyway, based of which options
> are implemented. Also we have seen a bit of people hiding
> controls by changing the access layer. Although sometimes that
> does result in things that are technically not class conformant
> but they do work through the Microsoft drivers, so we will likely
> need to support it anyway.

Sorry, I missed what the conclusion was, would this filter stay or be removed?

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Charles Keepax]

On Wed, May 14, 2025 at 02:39:20PM +0200, Pierre-Louis Bossart wrote:
> On 5/13/25 12:24, Charles Keepax wrote:
> > On Mon, May 12, 2025 at 06:14:00PM +0100, Charles Keepax wrote:
> >> On Mon, May 12, 2025 at 03:53:36PM +0200, Pierre-Louis Bossart wrote:
> >>>
> >>>> +static bool exported_control(struct sdca_control *control)
> >>>> +{
> >>>> +	/* No need to export control for something that only has one value */
> >>>> +	if (control->has_fixed)
> >>>> +		return false;
> >>>
> >>> why is that? It'd simplify userspace if there was always a way to
> >>> get a value, even if it's fixed?
> >>
> >> Well mostly it is because various things in the stack get
> >> sad with a single value control you can't set. If we wanted
> >> to change that I would suggest we add a FIXME. It isn't really
> >> critical and I would really like to get more of this class
> >> driver stuff moving, it has been out for review for a really long
> >> time now.
> >>
> > 
> > Thinking a little more about this user-space is going to have to
> > handle controls not being there as different controls will be
> > exported across different parts anyway, based of which options
> > are implemented. Also we have seen a bit of people hiding
> > controls by changing the access layer. Although sometimes that
> > does result in things that are technically not class conformant
> > but they do work through the Microsoft drivers, so we will likely
> > need to support it anyway.
> 
> Sorry, I missed what the conclusion was, would this filter stay or be removed?

Not sure I was clear either :-)

I have been poking this a little more today and I think I might
have overstated some of the problems here. I think it might be ok
to just mark the controls are read only. Will keep poking it and
will update soon, but kinda hopeful we could actually export
these in the next rev.

Thanks,
Charles

Re: [PATCH v5 5/6] ASoC: SDCA: Create ALSA controls from DisCo

[Pierre-Louis Bossart]

On 5/12/25 19:14, Charles Keepax wrote:
> On Mon, May 12, 2025 at 03:53:36PM +0200, Pierre-Louis Bossart wrote:
>>
>>> +static bool exported_control(struct sdca_control *control)
>>> +{
>>> +	/* No need to export control for something that only has one value */
>>> +	if (control->has_fixed)
>>> +		return false;
>>
>> why is that? It'd simplify userspace if there was always a way to
>> get a value, even if it's fixed?
> 
> Well mostly it is because various things in the stack get
> sad with a single value control you can't set. If we wanted
> to change that I would suggest we add a FIXME. It isn't really
> critical and I would really like to get more of this class
> driver stuff moving, it has been out for review for a really long
> time now.

Not following what parts of the stack you are referring to. We will have controls which are sometimes non-existent (DC), fixed (ROM) and some RW. Userspace has to be tolerant of those variations, no?

I am fine with the FIXME and I understand the frustration with limited reviews over a long time.


>>> @@ -99,6 +113,7 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
>>>  		case SDCA_ENTITY_TYPE_OT:
>>>  			*num_routes += !!entity->iot.clock;
>>>  			*num_routes += !!entity->iot.is_dataport;
>>> +			*num_controls += !entity->iot.is_dataport;
>>
>> shouldn't the same line be added for TYPE_IT?
> 
> This is just the cutting on the patch generation, case
> SDCA_ENTITY_TYPE_IT: is the line right above so this runs for
> both.

ok.

[PATCH v5 6/6] ASoC: SDCA: Create DAI drivers from DisCo

[Charles Keepax]

Use the previously parsed DisCo information from ACPI to create the DAI
drivers required to connect an SDCA Function into an ASoC soundcard.

Create DAI driver structures and populate the supported sample rates
and sample widths into them based on the Input/Output Terminal and any
attach Clock Source entities. More complex relationships with channels
etc. will be added later as constraints as part of the DAI startup.

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

Changes since v4:
 - Add include of soc-dai.h, this should be included directly.

 include/sound/sdca_asoc.h     |  12 +-
 include/sound/sdca_function.h |  23 ++++
 sound/soc/sdca/sdca_asoc.c    | 227 +++++++++++++++++++++++++++++++++-
 3 files changed, 256 insertions(+), 6 deletions(-)

diff --git a/include/sound/sdca_asoc.h b/include/sound/sdca_asoc.h
index d19e7e969283a..9121531f08260 100644
--- a/include/sound/sdca_asoc.h
+++ b/include/sound/sdca_asoc.h
@@ -14,11 +14,14 @@ struct device;
 struct sdca_function_data;
 struct snd_kcontrol_new;
 struct snd_soc_component_driver;
+struct snd_soc_dai_driver;
+struct snd_soc_dai_ops;
 struct snd_soc_dapm_route;
 struct snd_soc_dapm_widget;
 
 int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
-			      int *num_widgets, int *num_routes, int *num_controls);
+			      int *num_widgets, int *num_routes, int *num_controls,
+			      int *num_dais);
 
 int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *function,
 			    struct snd_soc_dapm_widget *widgets,
@@ -26,9 +29,14 @@ int sdca_asoc_populate_dapm(struct device *dev, struct sdca_function_data *funct
 int sdca_asoc_populate_controls(struct device *dev,
 				struct sdca_function_data *function,
 				struct snd_kcontrol_new *kctl);
+int sdca_asoc_populate_dais(struct device *dev, struct sdca_function_data *function,
+			    struct snd_soc_dai_driver *dais,
+			    const struct snd_soc_dai_ops *ops);
 
 int sdca_asoc_populate_component(struct device *dev,
 				 struct sdca_function_data *function,
-				 struct snd_soc_component_driver *component_drv);
+				 struct snd_soc_component_driver *component_drv,
+				 struct snd_soc_dai_driver **dai_drv, int *num_dai_drv,
+				 const struct snd_soc_dai_ops *ops);
 
 #endif // __SDCA_ASOC_H__
diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index 77ffb1f4e1ca9..be7e4a88cbed0 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -168,6 +168,20 @@ enum sdca_ot_controls {
 	SDCA_CTL_OT_NDAI_PACKETTYPE			= 0x17,
 };
 
+/**
+ * enum sdca_usage_range - Column definitions for Usage
+ */
+enum sdca_usage_range {
+	SDCA_USAGE_NUMBER				= 0,
+	SDCA_USAGE_CBN					= 1,
+	SDCA_USAGE_SAMPLE_RATE				= 2,
+	SDCA_USAGE_SAMPLE_WIDTH				= 3,
+	SDCA_USAGE_FULL_SCALE				= 4,
+	SDCA_USAGE_NOISE_FLOOR				= 5,
+	SDCA_USAGE_TAG					= 6,
+	SDCA_USAGE_NCOLS				= 7,
+};
+
 /**
  * enum sdca_mu_controls - SDCA Controls for Mixer Unit
  *
@@ -246,6 +260,15 @@ enum sdca_cs_controls {
 	SDCA_CTL_CS_SAMPLERATEINDEX			= 0x10,
 };
 
+/**
+ * enum sdca_samplerateindex_range - Column definitions for SampleRateIndex
+ */
+enum sdca_samplerateindex_range {
+	SDCA_SAMPLERATEINDEX_INDEX			= 0,
+	SDCA_SAMPLERATEINDEX_RATE			= 1,
+	SDCA_SAMPLERATEINDEX_NCOLS			= 2,
+};
+
 /**
  * enum sdca_cx_controls - SDCA Controls for Clock Selector
  *
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index e3debfe55eb04..c7db8c2750d75 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -20,6 +20,7 @@
 #include <sound/sdca_function.h>
 #include <sound/soc.h>
 #include <sound/soc-component.h>
+#include <sound/soc-dai.h>
 #include <sound/soc-dapm.h>
 #include <sound/tlv.h>
 
@@ -89,6 +90,8 @@ static bool exported_control(struct sdca_control *control)
  * required number of DAPM routes for the Function.
  * @num_controls: Output integer pointer, will be filled with the
  * required number of ALSA controls for the Function.
+ * @num_dais: Output integer pointer, will be filled with the
+ * required number of ASoC DAIs for the Function.
  *
  * This function counts various things within the SDCA Function such
  * that the calling driver can allocate appropriate space before
@@ -97,13 +100,15 @@ static bool exported_control(struct sdca_control *control)
  * Return: Returns zero on success, and a negative error code on failure.
  */
 int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *function,
-			      int *num_widgets, int *num_routes, int *num_controls)
+			      int *num_widgets, int *num_routes, int *num_controls,
+			      int *num_dais)
 {
 	int i, j;
 
 	*num_widgets = function->num_entities - 1;
 	*num_routes = 0;
 	*num_controls = 0;
+	*num_dais = 0;
 
 	for (i = 0; i < function->num_entities - 1; i++) {
 		struct sdca_entity *entity = &function->entities[i];
@@ -114,6 +119,7 @@ int sdca_asoc_count_component(struct device *dev, struct sdca_function_data *fun
 			*num_routes += !!entity->iot.clock;
 			*num_routes += !!entity->iot.is_dataport;
 			*num_controls += !entity->iot.is_dataport;
+			*num_dais += !!entity->iot.is_dataport;
 			break;
 		case SDCA_ENTITY_TYPE_PDE:
 			*num_routes += entity->pde.num_managed;
@@ -1013,6 +1019,205 @@ int sdca_asoc_populate_controls(struct device *dev,
 }
 EXPORT_SYMBOL_NS(sdca_asoc_populate_controls, "SND_SOC_SDCA");
 
+static unsigned int rate_find_mask(unsigned int rate)
+{
+	switch (rate) {
+	case 0:
+		return SNDRV_PCM_RATE_8000_768000;
+	case 5512:
+		return SNDRV_PCM_RATE_5512;
+	case 8000:
+		return SNDRV_PCM_RATE_8000;
+	case 11025:
+		return SNDRV_PCM_RATE_11025;
+	case 16000:
+		return SNDRV_PCM_RATE_16000;
+	case 22050:
+		return SNDRV_PCM_RATE_22050;
+	case 32000:
+		return SNDRV_PCM_RATE_32000;
+	case 44100:
+		return SNDRV_PCM_RATE_44100;
+	case 48000:
+		return SNDRV_PCM_RATE_48000;
+	case 64000:
+		return SNDRV_PCM_RATE_64000;
+	case 88200:
+		return SNDRV_PCM_RATE_88200;
+	case 96000:
+		return SNDRV_PCM_RATE_96000;
+	case 176400:
+		return SNDRV_PCM_RATE_176400;
+	case 192000:
+		return SNDRV_PCM_RATE_192000;
+	case 352800:
+		return SNDRV_PCM_RATE_352800;
+	case 384000:
+		return SNDRV_PCM_RATE_384000;
+	case 705600:
+		return SNDRV_PCM_RATE_705600;
+	case 768000:
+		return SNDRV_PCM_RATE_768000;
+	case 12000:
+		return SNDRV_PCM_RATE_12000;
+	case 24000:
+		return SNDRV_PCM_RATE_24000;
+	case 128000:
+		return SNDRV_PCM_RATE_128000;
+	default:
+		return 0;
+	}
+}
+
+static u64 width_find_mask(unsigned int bits)
+{
+	switch (bits) {
+	case 0:
+		return SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE |
+		       SNDRV_PCM_FMTBIT_S20_LE | SNDRV_PCM_FMTBIT_S24_LE |
+		       SNDRV_PCM_FMTBIT_S32_LE;
+	case 8:
+		return SNDRV_PCM_FMTBIT_S8;
+	case 16:
+		return SNDRV_PCM_FMTBIT_S16_LE;
+	case 20:
+		return SNDRV_PCM_FMTBIT_S20_LE;
+	case 24:
+		return SNDRV_PCM_FMTBIT_S24_LE;
+	case 32:
+		return SNDRV_PCM_FMTBIT_S32_LE;
+	default:
+		return 0;
+	}
+}
+
+static int populate_rate_format(struct device *dev,
+				struct sdca_function_data *function,
+				struct sdca_entity *entity,
+				struct snd_soc_pcm_stream *stream)
+{
+	struct sdca_control_range *range;
+	unsigned int sample_rate, sample_width;
+	unsigned int clock_rates = 0;
+	unsigned int rates = 0;
+	u64 formats = 0;
+	int sel, i;
+
+	switch (entity->type) {
+	case SDCA_ENTITY_TYPE_IT:
+		sel = SDCA_CTL_IT_USAGE;
+		break;
+	case SDCA_ENTITY_TYPE_OT:
+		sel = SDCA_CTL_OT_USAGE;
+		break;
+	default:
+		dev_err(dev, "%s: entity type has no usage control\n",
+			entity->label);
+		return -EINVAL;
+	}
+
+	if (entity->iot.clock) {
+		range = selector_find_range(dev, entity->iot.clock,
+					    SDCA_CTL_CS_SAMPLERATEINDEX,
+					    SDCA_SAMPLERATEINDEX_NCOLS, 0);
+		if (!range)
+			return -EINVAL;
+
+		for (i = 0; i < range->rows; i++) {
+			sample_rate = sdca_range(range, SDCA_SAMPLERATEINDEX_RATE, i);
+			clock_rates |= rate_find_mask(sample_rate);
+		}
+	} else {
+		clock_rates = UINT_MAX;
+	}
+
+	range = selector_find_range(dev, entity, sel, SDCA_USAGE_NCOLS, 0);
+	if (!range)
+		return -EINVAL;
+
+	for (i = 0; i < range->rows; i++) {
+		sample_rate = sdca_range(range, SDCA_USAGE_SAMPLE_RATE, i);
+		sample_rate = rate_find_mask(sample_rate);
+
+		if (sample_rate & clock_rates) {
+			rates |= sample_rate;
+
+			sample_width = sdca_range(range, SDCA_USAGE_SAMPLE_WIDTH, i);
+			formats |= width_find_mask(sample_width);
+		}
+	}
+
+	stream->formats = formats;
+	stream->rates = rates;
+
+	return 0;
+}
+
+/**
+ * sdca_asoc_populate_dais - fill in an array of DAI drivers for a Function
+ * @dev: Pointer to the device against which allocations will be done.
+ * @function: Pointer to the Function information.
+ * @dais: Array of DAI drivers to be populated.
+ * @ops: DAI ops to be attached to each of the created DAI drivers.
+ *
+ * This function populates an array of ASoC DAI drivers from the DisCo
+ * information for a particular SDCA Function. Typically,
+ * snd_soc_asoc_count_component will be used to allocate an
+ * appropriately sized array before calling this function.
+ *
+ * Return: Returns zero on success, and a negative error code on failure.
+ */
+
+int sdca_asoc_populate_dais(struct device *dev, struct sdca_function_data *function,
+			    struct snd_soc_dai_driver *dais,
+			    const struct snd_soc_dai_ops *ops)
+{
+	int i, j;
+	int ret;
+
+	for (i = 0, j = 0; i < function->num_entities - 1; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+		struct snd_soc_pcm_stream *stream;
+		const char *stream_suffix;
+
+		switch (entity->type) {
+		case SDCA_ENTITY_TYPE_IT:
+			stream = &dais[j].playback;
+			stream_suffix = "Playback";
+			break;
+		case SDCA_ENTITY_TYPE_OT:
+			stream = &dais[j].capture;
+			stream_suffix = "Capture";
+			break;
+		default:
+			continue;
+		}
+
+		if (!entity->iot.is_dataport)
+			continue;
+
+		stream->stream_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
+						     entity->label, stream_suffix);
+		if (!stream->stream_name)
+			return -ENOMEM;
+		/* Channels will be further limited by constraints */
+		stream->channels_min = 1;
+		stream->channels_max = SDCA_MAX_CHANNEL_COUNT;
+
+		ret = populate_rate_format(dev, function, entity, stream);
+		if (ret)
+			return ret;
+
+		dais[j].id = i;
+		dais[j].name = entity->label;
+		dais[j].ops = ops;
+		j++;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS(sdca_asoc_populate_dais, "SND_SOC_SDCA");
+
 /**
  * sdca_asoc_populate_component - fill in a component driver for a Function
  * @dev: Pointer to the device against which allocations will be done.
@@ -1027,16 +1232,19 @@ EXPORT_SYMBOL_NS(sdca_asoc_populate_controls, "SND_SOC_SDCA");
  */
 int sdca_asoc_populate_component(struct device *dev,
 				 struct sdca_function_data *function,
-				 struct snd_soc_component_driver *component_drv)
+				 struct snd_soc_component_driver *component_drv,
+				 struct snd_soc_dai_driver **dai_drv, int *num_dai_drv,
+				 const struct snd_soc_dai_ops *ops)
 {
 	struct snd_soc_dapm_widget *widgets;
 	struct snd_soc_dapm_route *routes;
 	struct snd_kcontrol_new *controls;
-	int num_widgets, num_routes, num_controls;
+	struct snd_soc_dai_driver *dais;
+	int num_widgets, num_routes, num_controls, num_dais;
 	int ret;
 
 	ret = sdca_asoc_count_component(dev, function, &num_widgets, &num_routes,
-					&num_controls);
+					&num_controls, &num_dais);
 	if (ret)
 		return ret;
 
@@ -1052,6 +1260,10 @@ int sdca_asoc_populate_component(struct device *dev,
 	if (!controls)
 		return -ENOMEM;
 
+	dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
+	if (!dais)
+		return -ENOMEM;
+
 	ret = sdca_asoc_populate_dapm(dev, function, widgets, routes);
 	if (ret)
 		return ret;
@@ -1060,6 +1272,10 @@ int sdca_asoc_populate_component(struct device *dev,
 	if (ret)
 		return ret;
 
+	ret = sdca_asoc_populate_dais(dev, function, dais, ops);
+	if (ret)
+		return ret;
+
 	component_drv->dapm_widgets = widgets;
 	component_drv->num_dapm_widgets = num_widgets;
 	component_drv->dapm_routes = routes;
@@ -1067,6 +1283,9 @@ int sdca_asoc_populate_component(struct device *dev,
 	component_drv->controls = controls;
 	component_drv->num_controls = num_controls;
 
+	*dai_drv = dais;
+	*num_dai_drv = num_dais;
+
 	return 0;
 }
 EXPORT_SYMBOL_NS(sdca_asoc_populate_component, "SND_SOC_SDCA");
-- 
2.39.5

Re: [PATCH v5 6/6] ASoC: SDCA: Create DAI drivers from DisCo

[Pierre-Louis Bossart]

> +int sdca_asoc_populate_dais(struct device *dev, struct sdca_function_data *function,
> +			    struct snd_soc_dai_driver *dais,
> +			    const struct snd_soc_dai_ops *ops)
> +{
> +	int i, j;
> +	int ret;
> +
> +	for (i = 0, j = 0; i < function->num_entities - 1; i++) {
> +		struct sdca_entity *entity = &function->entities[i];
> +		struct snd_soc_pcm_stream *stream;
> +		const char *stream_suffix;
> +
> +		switch (entity->type) {
> +		case SDCA_ENTITY_TYPE_IT:
> +			stream = &dais[j].playback;
> +			stream_suffix = "Playback";
> +			break;
> +		case SDCA_ENTITY_TYPE_OT:
> +			stream = &dais[j].capture;
> +			stream_suffix = "Capture";
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		if (!entity->iot.is_dataport)
> +			continue;

should this be moved as the first test in the for() loop? The switch cases for IT/OT only makes sense for streaming/dataport terminals, no?

> +
> +		stream->stream_name = devm_kasprintf(dev, GFP_KERNEL, "%s %s",
> +						     entity->label, stream_suffix);
> +		if (!stream->stream_name)
> +			return -ENOMEM;
> +		/* Channels will be further limited by constraints */
> +		stream->channels_min = 1;
> +		stream->channels_max = SDCA_MAX_CHANNEL_COUNT;
> +
> +		ret = populate_rate_format(dev, function, entity, stream);
> +		if (ret)
> +			return ret;
> +
> +		dais[j].id = i;
> +		dais[j].name = entity->label;
> +		dais[j].ops = ops;
> +		j++;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(sdca_asoc_populate_dais, "SND_SOC_SDCA");

Re: [PATCH v5 6/6] ASoC: SDCA: Create DAI drivers from DisCo

[Charles Keepax]

On Mon, May 12, 2025 at 04:00:18PM +0200, Pierre-Louis Bossart wrote:
> > +		switch (entity->type) {
> > +		case SDCA_ENTITY_TYPE_IT:
> > +			stream = &dais[j].playback;
> > +			stream_suffix = "Playback";
> > +			break;
> > +		case SDCA_ENTITY_TYPE_OT:
> > +			stream = &dais[j].capture;
> > +			stream_suffix = "Capture";
> > +			break;
> > +		default:
> > +			continue;
> > +		}
> > +
> > +		if (!entity->iot.is_dataport)
> > +			continue;
> 
> should this be moved as the first test in the for() loop? The switch
> cases for IT/OT only makes sense for streaming/dataport terminals, no?

Can't be first in the loop since the entities can be different
types and the iot is part of a union so it is only safe to access
is_dataport once you know it is a terminal entity.

Thanks,
Charles

Re: [PATCH v5 6/6] ASoC: SDCA: Create DAI drivers from DisCo

[Pierre-Louis Bossart]

On 5/12/25 19:16, Charles Keepax wrote:
> On Mon, May 12, 2025 at 04:00:18PM +0200, Pierre-Louis Bossart wrote:
>>> +		switch (entity->type) {
>>> +		case SDCA_ENTITY_TYPE_IT:
>>> +			stream = &dais[j].playback;
>>> +			stream_suffix = "Playback";
>>> +			break;
>>> +		case SDCA_ENTITY_TYPE_OT:
>>> +			stream = &dais[j].capture;
>>> +			stream_suffix = "Capture";
>>> +			break;
>>> +		default:
>>> +			continue;
>>> +		}
>>> +
>>> +		if (!entity->iot.is_dataport)
>>> +			continue;
>>
>> should this be moved as the first test in the for() loop? The switch
>> cases for IT/OT only makes sense for streaming/dataport terminals, no?
> 
> Can't be first in the loop since the entities can be different
> types and the iot is part of a union so it is only safe to access
> is_dataport once you know it is a terminal entity.

ok, it's be nice to add a comment for the rest of us who don't remember the low-level details of the data structures :-)

Re: [PATCH v5 6/6] ASoC: SDCA: Create DAI drivers from DisCo

[Charles Keepax]

On Wed, May 14, 2025 at 02:38:11PM +0200, Pierre-Louis Bossart wrote:
> On 5/12/25 19:16, Charles Keepax wrote:
> > On Mon, May 12, 2025 at 04:00:18PM +0200, Pierre-Louis Bossart wrote:
> >>> +		switch (entity->type) {
> >>> +		case SDCA_ENTITY_TYPE_IT:
> >>> +			stream = &dais[j].playback;
> >>> +			stream_suffix = "Playback";
> >>> +			break;
> >>> +		case SDCA_ENTITY_TYPE_OT:
> >>> +			stream = &dais[j].capture;
> >>> +			stream_suffix = "Capture";
> >>> +			break;
> >>> +		default:
> >>> +			continue;
> >>> +		}
> >>> +
> >>> +		if (!entity->iot.is_dataport)
> >>> +			continue;
> >>
> >> should this be moved as the first test in the for() loop? The switch
> >> cases for IT/OT only makes sense for streaming/dataport terminals, no?
> > 
> > Can't be first in the loop since the entities can be different
> > types and the iot is part of a union so it is only safe to access
> > is_dataport once you know it is a terminal entity.
> 
> ok, it's be nice to add a comment for the rest of us who
> don't remember the low-level details of the data structures :-)

Yeah never mind putting a comment in, I will add one for the next
spin.

Thanks,
Charles