[PATCH 0/7] Add SDCA IRQ support and some misc fixups

[PATCH 0/7] Add SDCA IRQ support and some misc fixups

[Charles Keepax]

Add a maintainers entry for SDCA, do a couple of small fixups for
previous chains, and then adding the beginnings of the SDCA IRQ
handling. This is based around a regmap IRQ chip and a few helper
functions that can be called from the client drivers to setup the
IRQs.

Thanks,
Charles

Charles Keepax (6):
  MAINTAINERS: Add SDCA maintainers entry
  ASoC: SDCA: Add missing default in switch in entity_pde_event()
  ASoC: SDCA: Fixup some kernel doc errors
  ASoC: SDCA: Minor selected/detected mode control fixups
  ASoC: SDCA: Add flag for unused IRQs
  ASoC: SDCA: Add some initial IRQ handlers

Maciej Strozek (1):
  ASoC: SDCA: Generic interrupt support

 MAINTAINERS                      |  11 +
 include/sound/sdca_function.h    |  11 +
 include/sound/sdca_interrupts.h  |  77 ++++++
 sound/soc/sdca/Kconfig           |   7 +
 sound/soc/sdca/Makefile          |   2 +
 sound/soc/sdca/sdca_asoc.c       |  19 +-
 sound/soc/sdca/sdca_functions.c  |   2 +
 sound/soc/sdca/sdca_interrupts.c | 437 +++++++++++++++++++++++++++++++
 8 files changed, 560 insertions(+), 6 deletions(-)
 create mode 100644 include/sound/sdca_interrupts.h
 create mode 100644 sound/soc/sdca/sdca_interrupts.c

-- 
2.39.5

[PATCH 1/7] MAINTAINERS: Add SDCA maintainers entry

[Charles Keepax]

Add a maintainers entry for the new SDCA support code.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c95f48f6635ad..50eb8b3cb2682 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21890,6 +21890,17 @@ M:	Jim Cromie <jim.cromie@gmail.com>
 S:	Maintained
 F:	drivers/clocksource/scx200_hrt.c
 
+SDCA LIBRARY AND CLASS DRIVER
+M:	Charles Keepax <ckeepax@opensource.cirrus.com>
+M:	Maciej Strozek <mstrozek@opensource.cirrus.com>
+R:	Bard Liao <yung-chuan.liao@linux.intel.com>
+R:	Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
+L:	linux-sound@vger.kernel.org
+L:	patches@opensource.cirrus.com
+S:	Maintained
+F:	include/sound/sdca*
+F:	sound/soc/sdca/*
+
 SDRICOH_CS MMC/SD HOST CONTROLLER INTERFACE DRIVER
 M:	Sascha Sommer <saschasommer@freenet.de>
 L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
-- 
2.39.5

[PATCH 2/7] ASoC: SDCA: Add missing default in switch in entity_pde_event()

[Charles Keepax]

The current code should be safe as the PDE widget only registers for the
two events handled in the switch statement. However, it is causing a
smatch warning and also is a little fragile to future code changes, add
a default case to avoid the warning and make the code more robust.

Fixes: 2c8b3a8e6aa8 ("ASoC: SDCA: Create DAPM widgets and routes from DisCo")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_asoc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index 7bc8f6069f3d4..e96e696cb1079 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -397,6 +397,8 @@ static int entity_pde_event(struct snd_soc_dapm_widget *widget,
 		from = widget->off_val;
 		to = widget->on_val;
 		break;
+	default:
+		return 0;
 	}
 
 	for (i = 0; i < entity->pde.num_max_delay; i++) {
-- 
2.39.5

[PATCH 3/7] ASoC: SDCA: Fixup some kernel doc errors

[Charles Keepax]

Correct some typos and omissions in the kernel doc for the ASoC SDCA
code.

Fixes: 2c8b3a8e6aa8 ("ASoC: SDCA: Create DAPM widgets and routes from DisCo")
Reported-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/sdca/sdca_asoc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index e96e696cb1079..83911dab73ae2 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -93,6 +93,7 @@ static bool readonly_control(struct sdca_control *control)
 
 /**
  * sdca_asoc_count_component - count the various component parts
+ * @dev: Pointer to the device against which allocations will be done.
  * @function: Pointer to the Function information.
  * @num_widgets: Output integer pointer, will be filled with the
  * required number of DAPM widgets for the Function.
@@ -997,7 +998,7 @@ static int populate_pin_switch(struct device *dev,
  * 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.
+ * @kctl: 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,
@@ -1244,7 +1245,11 @@ 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.
  * @function: Pointer to the Function information.
- * @copmonent_drv: Pointer to the component driver to be populated.
+ * @component_drv: Pointer to the component driver to be populated.
+ * @dai_drv: Pointer to the DAI driver array to be allocated and populated.
+ * @num_dai_drv: Pointer to integer that will be populated with the number of
+ * DAI drivers.
+ * @ops: DAI ops pointer that will be used for each DAI driver.
  *
  * This function populates a snd_soc_component_driver structure based
  * on the DisCo information for a particular SDCA Function. It does
-- 
2.39.5

[PATCH 4/7] ASoC: SDCA: Minor selected/detected mode control fixups

[Charles Keepax]

Make the names a slightly better match for the specification and add
some constants for the values rather than hard coding.

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

diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index eaedb54a83227..e5166583d2581 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -318,6 +318,15 @@ enum sdca_selected_mode_range {
 	SDCA_SELECTED_MODE_NCOLS			= 2,
 };
 
+/**
+ * enum sdca_detected_mode_values - Predefined GE Detected Mode values
+ */
+enum sdca_detected_mode_values {
+	SDCA_DETECTED_MODE_JACK_UNPLUGGED		= 0,
+	SDCA_DETECTED_MODE_JACK_UNKNOWN			= 1,
+	SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS	= 2,
+};
+
 /**
  * enum sdca_spe_controls - SDCA Controls for Security & Privacy Unit
  *
diff --git a/sound/soc/sdca/sdca_asoc.c b/sound/soc/sdca/sdca_asoc.c
index 83911dab73ae2..dd7b19083c85f 100644
--- a/sound/soc/sdca/sdca_asoc.c
+++ b/sound/soc/sdca/sdca_asoc.c
@@ -246,12 +246,12 @@ static int entity_early_parse_ge(struct device *dev,
 	if (!values)
 		return -ENOMEM;
 
-	texts[0] = "No Jack";
+	texts[0] = "Jack Unplugged";
 	texts[1] = "Jack Unknown";
 	texts[2] = "Detection in Progress";
-	values[0] = 0;
-	values[1] = 1;
-	values[2] = 2;
+	values[0] = SDCA_DETECTED_MODE_JACK_UNPLUGGED;
+	values[1] = SDCA_DETECTED_MODE_JACK_UNKNOWN;
+	values[2] = SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS;
 	for (i = 0; i < range->rows; i++) {
 		enum sdca_terminal_type type;
 
-- 
2.39.5

[PATCH 5/7] ASoC: SDCA: Add flag for unused IRQs

[Charles Keepax]

Zero is a valid SDCA IRQ interrupt position so add a special value to
indicate that the IRQ is not used.

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

diff --git a/include/sound/sdca_function.h b/include/sound/sdca_function.h
index e5166583d2581..f208d8f03db4e 100644
--- a/include/sound/sdca_function.h
+++ b/include/sound/sdca_function.h
@@ -16,6 +16,8 @@ struct device;
 struct sdca_entity;
 struct sdca_function_desc;
 
+#define SDCA_NO_INTERRUPT -1
+
 /*
  * The addressing space for SDCA relies on 7 bits for Entities, so a
  * maximum of 128 Entities per function can be represented.
diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 64ac264438907..3afe9a24e4e45 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -911,6 +911,8 @@ static int find_sdca_entity_control(struct device *dev, struct sdca_entity *enti
 				       &tmp);
 	if (!ret)
 		control->interrupt_position = tmp;
+	else
+		control->interrupt_position = SDCA_NO_INTERRUPT;
 
 	control->label = find_sdca_control_label(dev, entity, control);
 	if (!control->label)
-- 
2.39.5

[PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Charles Keepax]

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

Add a library supporting usage of SDCA interrupts, using regmap irq
framework. The library adds functions for parsing ACPI for
interrupt-related information, configuring irq chip and requesting
individual irqs. Calling code (SDCA function code) is expected to also
substitute the library's base irq handler for its own, appropriate
callback.

Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/sdca_interrupts.h  |  74 ++++++++
 sound/soc/sdca/Kconfig           |   7 +
 sound/soc/sdca/Makefile          |   2 +
 sound/soc/sdca/sdca_interrupts.c | 284 +++++++++++++++++++++++++++++++
 4 files changed, 367 insertions(+)
 create mode 100644 include/sound/sdca_interrupts.h
 create mode 100644 sound/soc/sdca/sdca_interrupts.c

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
new file mode 100644
index 0000000000000..10c14db282bfe
--- /dev/null
+++ b/include/sound/sdca_interrupts.h
@@ -0,0 +1,74 @@
+/* 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_INTERRUPTS_H__
+#define __SDCA_INTERRUPTS_H__
+
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+struct device;
+struct snd_soc_component;
+struct sdca_function_data;
+
+#define SDCA_MAX_INTERRUPTS 31 /* the last bit is reserved for future extensions */
+
+/**
+ * struct sdca_interrupt - contains information about a single SDCA interrupt
+ * @name: The name of the interrupt.
+ * @component: Pointer to the ASoC component owns the interrupt.
+ * @function: Pointer to the Function that the interrupt is associated with.
+ * @entity: Pointer to the Entity that the interrupt is associated with.
+ * @control: Pointer to the Control that the interrupt is associated with.
+ * @externally_requested: Internal flag used to check if something has already
+ * requested the interrupt.
+ */
+struct sdca_interrupt {
+	const char *name;
+
+	struct snd_soc_component *component;
+	struct sdca_function_data *function;
+	struct sdca_entity *entity;
+	struct sdca_control *control;
+
+	bool externally_requested;
+};
+
+/**
+ * struct sdca_interrupt_info - contains top-level SDCA interrupt information
+ * @irq_chip: regmap irq chip structure.
+ * @irq_data: regmap irq chip data structure.
+ * @irqs: Array of data for each individual IRQ.
+ * @irq_lock: Protects access to the list of sdca_interrupt structures.
+ */
+struct sdca_interrupt_info {
+	struct regmap_irq_chip irq_chip;
+	struct regmap_irq_chip_data *irq_data;
+
+	struct sdca_interrupt irqs[SDCA_MAX_INTERRUPTS];
+
+	struct mutex irq_lock; /* Protect accesses to irqs list */
+};
+
+int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *interrupt_info,
+		     int sdca_irq, const char *name, irq_handler_t handler,
+		     void *data);
+int sdca_irq_data_populate(struct snd_soc_component *component,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct sdca_control *control,
+			   struct sdca_interrupt *interrupt);
+int sdca_irq_populate(struct sdca_function_data *function,
+		      struct snd_soc_component *component,
+		      struct sdca_interrupt_info *info);
+struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
+					      struct regmap *regmap, int irq);
+
+#endif
diff --git a/sound/soc/sdca/Kconfig b/sound/soc/sdca/Kconfig
index ee20b9914aa1f..d40331cb0a212 100644
--- a/sound/soc/sdca/Kconfig
+++ b/sound/soc/sdca/Kconfig
@@ -9,3 +9,10 @@ config SND_SOC_SDCA
 
 config SND_SOC_SDCA_OPTIONAL
 	def_tristate SND_SOC_SDCA || !SND_SOC_SDCA
+
+config SND_SOC_SDCA_IRQ
+	tristate
+	select REGMAP
+	select REGMAP_IRQ
+	help
+	  This option enables support for SDCA IRQs.
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index 53344f108ca67..d7f9a16790ad8 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 snd-soc-sdca-y	:= sdca_functions.o sdca_device.o sdca_regmap.o sdca_asoc.o
+snd-soc-sdca-irq-y := sdca_interrupts.o
 
 obj-$(CONFIG_SND_SOC_SDCA)	+= snd-soc-sdca.o
+obj-$(CONFIG_SND_SOC_SDCA_IRQ) += snd-soc-sdca-irq.o
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
new file mode 100644
index 0000000000000..1b02d584cb5a3
--- /dev/null
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -0,0 +1,284 @@
+// 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/bits.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <sound/sdca.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/soc-component.h>
+
+#define IRQ_SDCA(number) REGMAP_IRQ_REG(number, ((number) / BITS_PER_BYTE), \
+					SDW_SCP_SDCA_INTMASK_SDCA_##number)
+
+static const struct regmap_irq regmap_irqs[SDCA_MAX_INTERRUPTS] = {
+	IRQ_SDCA(0),
+	IRQ_SDCA(1),
+	IRQ_SDCA(2),
+	IRQ_SDCA(3),
+	IRQ_SDCA(4),
+	IRQ_SDCA(5),
+	IRQ_SDCA(6),
+	IRQ_SDCA(7),
+	IRQ_SDCA(8),
+	IRQ_SDCA(9),
+	IRQ_SDCA(10),
+	IRQ_SDCA(11),
+	IRQ_SDCA(12),
+	IRQ_SDCA(13),
+	IRQ_SDCA(14),
+	IRQ_SDCA(15),
+	IRQ_SDCA(16),
+	IRQ_SDCA(17),
+	IRQ_SDCA(18),
+	IRQ_SDCA(19),
+	IRQ_SDCA(20),
+	IRQ_SDCA(21),
+	IRQ_SDCA(22),
+	IRQ_SDCA(23),
+	IRQ_SDCA(24),
+	IRQ_SDCA(25),
+	IRQ_SDCA(26),
+	IRQ_SDCA(27),
+	IRQ_SDCA(28),
+	IRQ_SDCA(29),
+	IRQ_SDCA(30),
+};
+
+static const struct regmap_irq_chip sdca_irq_chip = {
+	.name = "sdca_irq",
+
+	.status_base = SDW_SCP_SDCA_INT1,
+	.unmask_base = SDW_SCP_SDCA_INTMASK1,
+	.ack_base = SDW_SCP_SDCA_INT1,
+	.num_regs = 4,
+
+	.irqs = regmap_irqs,
+	.num_irqs = SDCA_MAX_INTERRUPTS,
+
+	.runtime_pm = true,
+};
+
+static irqreturn_t base_handler(int irq, void *data)
+{
+	struct sdca_interrupt *interrupt = data;
+	struct device *dev = interrupt->component->dev;
+
+	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
+
+	return IRQ_HANDLED;
+}
+
+static int sdca_irq_request_locked(struct device *dev,
+				   struct sdca_interrupt_info *info,
+				   int sdca_irq, const char *name,
+				   irq_handler_t handler, void *data)
+{
+	int irq;
+	int ret;
+
+	irq = regmap_irq_get_virq(info->irq_data, sdca_irq);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, handler,
+					IRQF_ONESHOT, name, data);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "requested irq %d for %s\n", irq, name);
+
+	return 0;
+}
+
+/**
+ * sdca_request_irq - request an individual SDCA interrupt
+ * @dev: Pointer to the struct device against which things should be allocated.
+ * @interrupt_info: Pointer to the interrupt information structure.
+ * @sdca_irq: SDCA interrupt position.
+ * @name: Name to be given to the IRQ.
+ * @handler: A callback thread function to be called for the IRQ.
+ * @data: Private data pointer that will be passed to the handler.
+ *
+ * Typically this is handled internally by sdca_irq_populate, however if
+ * a device requires custom IRQ handling this can be called manually before
+ * calling sdca_irq_populate, which will then skip that IRQ whilst processing.
+ *
+ * Return: Zero on success, and a negative error code on failure.
+ */
+int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
+		     int sdca_irq, const char *name, irq_handler_t handler,
+		     void *data)
+{
+	int ret;
+
+	if (sdca_irq < 0 || sdca_irq > SDCA_MAX_INTERRUPTS) {
+		dev_err(dev, "bad irq request: %d\n", sdca_irq);
+		return -EINVAL;
+	}
+
+	guard(mutex)(&info->irq_lock);
+
+	ret = sdca_irq_request_locked(dev, info, sdca_irq, name, handler, data);
+	if (ret) {
+		dev_err(dev, "failed to request irq %s: %d\n", name, ret);
+		return ret;
+	}
+
+	info->irqs[sdca_irq].externally_requested = true;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA_IRQ");
+
+/**
+ * sdca_irq_data_populate - Populate common interrupt data
+ * @component: Pointer to the ASoC component for the Function.
+ * @function: Pointer to the SDCA Function.
+ * @entity: Pointer to the SDCA Entity.
+ * @control: Pointer to the SDCA Control.
+ * @interrupt: Pointer to the SDCA interrupt for this IRQ.
+ *
+ * Return: Zero on success, and a negative error code on failure.
+ */
+int sdca_irq_data_populate(struct snd_soc_component *component,
+			   struct sdca_function_data *function,
+			   struct sdca_entity *entity,
+			   struct sdca_control *control,
+			   struct sdca_interrupt *interrupt)
+{
+	struct device *dev = component->dev;
+	const char *name;
+
+	name = devm_kasprintf(dev, GFP_KERNEL, "%s %s %s", function->desc->name,
+			      entity->label, control->label);
+	if (!name)
+		return -ENOMEM;
+
+	interrupt->name = name;
+	interrupt->component = component;
+	interrupt->function = function;
+	interrupt->entity = entity;
+	interrupt->control = control;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_data_populate, "SND_SOC_SDCA_IRQ");
+
+/**
+ * sdca_irq_populate - Request all the individual IRQs for an SDCA Function
+ * @function: Pointer to the SDCA Function.
+ * @component: Pointer to the ASoC component for the Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ *
+ * Typically this would be called from the driver for a single SDCA Function.
+ *
+ * Return: Zero on success, and a negative error code on failure.
+ */
+int sdca_irq_populate(struct sdca_function_data *function,
+		      struct snd_soc_component *component,
+		      struct sdca_interrupt_info *info)
+{
+	struct device *dev = component->dev;
+	int i, j;
+
+	guard(mutex)(&info->irq_lock);
+
+	for (i = 0; i < function->num_entities; i++) {
+		struct sdca_entity *entity = &function->entities[i];
+
+		for (j = 0; j < entity->num_controls; j++) {
+			struct sdca_control *control = &entity->controls[j];
+			int irq = control->interrupt_position;
+			struct sdca_interrupt *interrupt;
+			const char *name;
+			int ret;
+
+			if (irq == SDCA_NO_INTERRUPT) {
+				continue;
+			} else if (irq < 0 || irq >= SDCA_MAX_INTERRUPTS) {
+				dev_err(dev, "bad irq position: %d\n", irq);
+				return -EINVAL;
+			}
+
+			interrupt = &info->irqs[irq];
+
+			if (interrupt->externally_requested) {
+				dev_dbg(dev,
+					"skipping irq %d, externally requested\n",
+					irq);
+				continue;
+			}
+
+			ret = sdca_irq_data_populate(component, function, entity,
+						     control, interrupt);
+			if (ret)
+				return ret;
+
+			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
+						      base_handler, interrupt);
+			if (ret) {
+				dev_err(dev, "failed to request irq %s: %d\n",
+					name, ret);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_populate, "SND_SOC_SDCA_IRQ");
+
+/**
+ * sdca_irq_allocate - allocate an SDCA interrupt structure for a device
+ * @dev: Device pointer against which things should be allocated.
+ * @regmap: regmap to be used for accessing the SDCA IRQ registers.
+ * @irq: The interrupt number.
+ *
+ * Typically this would be called from the top level driver for the whole
+ * SDCA device, as only a single instance is required across all Functions
+ * on the device.
+ *
+ * Return: A pointer to the allocated sdca_interrupt_info struct, or an
+ * error code.
+ */
+struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
+					      struct regmap *regmap, int irq)
+{
+	struct sdca_interrupt_info *info;
+	int ret;
+
+	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	info->irq_chip = sdca_irq_chip;
+
+	devm_mutex_init(dev, &info->irq_lock);
+
+	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
+				       &info->irq_chip, &info->irq_data);
+	if (ret) {
+		dev_err(dev, "failed to register irq chip: %d\n", ret);
+		return ERR_PTR(ret);
+	}
+
+	dev_dbg(dev, "registered on irq %d\n", irq);
+
+	return info;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_allocate, "SND_SOC_SDCA_IRQ");
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SDCA IRQ library");
-- 
2.39.5

[PATCH 7/7] ASoC: SDCA: Add some initial IRQ handlers

[Charles Keepax]

Add basic IRQ handlers for the function status and jack detection
interrupts.

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

diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index 10c14db282bfe..a688197896784 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -27,6 +27,7 @@ struct sdca_function_data;
  * @function: Pointer to the Function that the interrupt is associated with.
  * @entity: Pointer to the Entity that the interrupt is associated with.
  * @control: Pointer to the Control that the interrupt is associated with.
+ * @priv: Pointer to private data for use by the handler.
  * @externally_requested: Internal flag used to check if something has already
  * requested the interrupt.
  */
@@ -38,6 +39,8 @@ struct sdca_interrupt {
 	struct sdca_entity *entity;
 	struct sdca_control *control;
 
+	void *priv;
+
 	bool externally_requested;
 };
 
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 1b02d584cb5a3..005b7a085a491 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -7,6 +7,7 @@
  * https://www.mipi.org/mipi-sdca-v1-0-download
  */
 
+#include <linux/bitmap.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
@@ -18,6 +19,7 @@
 #include <sound/sdca_function.h>
 #include <sound/sdca_interrupts.h>
 #include <sound/soc-component.h>
+#include <sound/soc.h>
 
 #define IRQ_SDCA(number) REGMAP_IRQ_REG(number, ((number) / BITS_PER_BYTE), \
 					SDW_SCP_SDCA_INTMASK_SDCA_##number)
@@ -80,6 +82,141 @@ static irqreturn_t base_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t function_status_handler(int irq, void *data)
+{
+	struct sdca_interrupt *interrupt = data;
+	struct device *dev = interrupt->component->dev;
+	unsigned int reg, val;
+	unsigned long status;
+	unsigned int mask;
+	int ret;
+
+	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
+			   interrupt->control->sel, 0);
+
+	ret = regmap_read(interrupt->component->regmap, reg, &val);
+	if (ret < 0) {
+		dev_err(dev, "failed to read function status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	dev_dbg(dev, "function status: %#x\n", val);
+
+	status = val;
+	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
+		mask = 1 << mask;
+
+		switch (mask) {
+		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
+			//FIXME: Add init writes
+			break;
+		case SDCA_CTL_ENTITY_0_FUNCTION_FAULT:
+			dev_err(dev, "function fault\n");
+			break;
+		case SDCA_CTL_ENTITY_0_UMP_SEQUENCE_FAULT:
+			dev_err(dev, "ump sequence fault\n");
+			break;
+		case SDCA_CTL_ENTITY_0_DEVICE_NEWLY_ATTACHED:
+		case SDCA_CTL_ENTITY_0_INTS_DISABLED_ABNORMALLY:
+		case SDCA_CTL_ENTITY_0_STREAMING_STOPPED_ABNORMALLY:
+		case SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET:
+		case SDCA_CTL_ENTITY_0_FUNCTION_BUSY:
+			break;
+		}
+	}
+
+	ret = regmap_write(interrupt->component->regmap, reg, val);
+	if (ret < 0) {
+		dev_err(dev, "failed to clear function status: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t detected_mode_handler(int irq, void *data)
+{
+	struct sdca_interrupt *interrupt = data;
+	struct snd_soc_component *component = interrupt->component;
+	struct device *dev = component->dev;
+	struct snd_soc_card *card = component->card;
+	struct rw_semaphore *rwsem = &card->snd_card->controls_rwsem;
+	struct snd_kcontrol *kctl = interrupt->priv;
+	struct snd_ctl_elem_value ucontrol;
+	struct soc_enum *soc_enum;
+	unsigned int reg, val;
+	int ret;
+
+	if (!kctl) {
+		const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s %s",
+							   interrupt->entity->label,
+							   SDCA_CTL_SELECTED_MODE_NAME);
+
+		if (!name)
+			return -ENOMEM;
+
+		kctl = snd_soc_component_get_kcontrol(component, name);
+		if (!kctl) {
+			dev_dbg(dev, "control not found: %s\n", name);
+			return IRQ_NONE;
+		}
+
+		interrupt->priv = kctl;
+	}
+
+	soc_enum = (struct soc_enum *)kctl->private_value;
+
+	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
+			   interrupt->control->sel, 0);
+
+	ret = regmap_read(component->regmap, reg, &val);
+	if (ret < 0) {
+		dev_err(dev, "failed to read detected mode: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	switch (val) {
+	case SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS:
+	case SDCA_DETECTED_MODE_JACK_UNKNOWN:
+		reg = SDW_SDCA_CTL(interrupt->function->desc->adr,
+				   interrupt->entity->id,
+				   SDCA_CTL_GE_SELECTED_MODE, 0);
+
+		/*
+		 * Selected mode is not typically a volatile register, but
+		 * force a read from the hardware in the case detected mode
+		 * is unknown to see what the device selected as a "safe"
+		 * option.
+		 */
+		regcache_drop_region(component->regmap, reg, reg);
+
+		ret = regmap_read(component->regmap, reg, &val);
+		if (ret) {
+			dev_err(dev, "failed to re-check selected mode: %d\n", ret);
+			return IRQ_NONE;
+		}
+		break;
+	default:
+		break;
+	}
+
+	dev_dbg(dev, "%s: %#x\n", interrupt->name, val);
+
+	ucontrol.value.enumerated.item[0] = snd_soc_enum_val_to_item(soc_enum, val);
+
+	down_write(rwsem);
+	ret = kctl->put(kctl, &ucontrol);
+	up_write(rwsem);
+	if (ret < 0) {
+		dev_err(dev, "failed to update selected mode: %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
+
+	return IRQ_HANDLED;
+}
+
 static int sdca_irq_request_locked(struct device *dev,
 				   struct sdca_interrupt_info *info,
 				   int sdca_irq, const char *name,
@@ -202,6 +339,7 @@ int sdca_irq_populate(struct sdca_function_data *function,
 			struct sdca_control *control = &entity->controls[j];
 			int irq = control->interrupt_position;
 			struct sdca_interrupt *interrupt;
+			irq_handler_t handler;
 			const char *name;
 			int ret;
 
@@ -226,8 +364,23 @@ int sdca_irq_populate(struct sdca_function_data *function,
 			if (ret)
 				return ret;
 
+			handler = base_handler;
+
+			switch (entity->type) {
+			case SDCA_ENTITY_TYPE_ENTITY_0:
+				if (control->sel == SDCA_CTL_ENTITY_0_FUNCTION_STATUS)
+					handler = function_status_handler;
+				break;
+			case SDCA_ENTITY_TYPE_GE:
+				if (control->sel == SDCA_CTL_GE_DETECTED_MODE)
+					handler = detected_mode_handler;
+				break;
+			default:
+				break;
+			}
+
 			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
-						      base_handler, interrupt);
+						      handler, interrupt);
 			if (ret) {
 				dev_err(dev, "failed to request irq %s: %d\n",
 					name, ret);
-- 
2.39.5

Re: [PATCH 0/7] Add SDCA IRQ support and some misc fixups

[Liao, Bard]

On 6/9/2025 8:39 PM, Charles Keepax wrote:
> Add a maintainers entry for SDCA, do a couple of small fixups for
> previous chains, and then adding the beginnings of the SDCA IRQ
> handling. This is based around a regmap IRQ chip and a few helper
> functions that can be called from the client drivers to setup the
> IRQs.
> 
> Thanks,
> Charles
> 
> Charles Keepax (6):
>   MAINTAINERS: Add SDCA maintainers entry
>   ASoC: SDCA: Add missing default in switch in entity_pde_event()
>   ASoC: SDCA: Fixup some kernel doc errors
>   ASoC: SDCA: Minor selected/detected mode control fixups
>   ASoC: SDCA: Add flag for unused IRQs
>   ASoC: SDCA: Add some initial IRQ handlers
> 
> Maciej Strozek (1):
>   ASoC: SDCA: Generic interrupt support
> 
>  MAINTAINERS                      |  11 +
>  include/sound/sdca_function.h    |  11 +
>  include/sound/sdca_interrupts.h  |  77 ++++++
>  sound/soc/sdca/Kconfig           |   7 +
>  sound/soc/sdca/Makefile          |   2 +
>  sound/soc/sdca/sdca_asoc.c       |  19 +-
>  sound/soc/sdca/sdca_functions.c  |   2 +
>  sound/soc/sdca/sdca_interrupts.c | 437 +++++++++++++++++++++++++++++++
>  8 files changed, 560 insertions(+), 6 deletions(-)
>  create mode 100644 include/sound/sdca_interrupts.h
>  create mode 100644 sound/soc/sdca/sdca_interrupts.c
> 

For this series
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Pierre-Louis Bossart]

> +/**
> + * struct sdca_interrupt - contains information about a single SDCA interrupt
> + * @name: The name of the interrupt.
> + * @component: Pointer to the ASoC component owns the interrupt.
> + * @function: Pointer to the Function that the interrupt is associated with.
> + * @entity: Pointer to the Entity that the interrupt is associated with.
> + * @control: Pointer to the Control that the interrupt is associated with.
> + * @externally_requested: Internal flag used to check if something has already
> + * requested the interrupt.

I am not following what 'externally' and 'something' refer to. Each interrupt is allocated to a specific function/entity/control, this hints at another agent getting in the way but that's not really how this is supposed to work.

This deserves additional clarifications IMHO.

> + */
> +struct sdca_interrupt {
> +	const char *name;
> +
> +	struct snd_soc_component *component;
> +	struct sdca_function_data *function;
> +	struct sdca_entity *entity;
> +	struct sdca_control *control;
> +
> +	bool externally_requested;
> +};
> +
> +/**
> + * struct sdca_interrupt_info - contains top-level SDCA interrupt information
> + * @irq_chip: regmap irq chip structure.
> + * @irq_data: regmap irq chip data structure.
> + * @irqs: Array of data for each individual IRQ.
> + * @irq_lock: Protects access to the list of sdca_interrupt structures.
> + */
> +struct sdca_interrupt_info {
> +	struct regmap_irq_chip irq_chip;
> +	struct regmap_irq_chip_data *irq_data;
> +
> +	struct sdca_interrupt irqs[SDCA_MAX_INTERRUPTS];
> +
> +	struct mutex irq_lock; /* Protect accesses to irqs list */

Can you elaborate on what might conflict?

The only thing I can think of in terms of required locking is access to the common SDCA interrupt registers, but that's different to the irq list.

> +static const struct regmap_irq_chip sdca_irq_chip = {
> +	.name = "sdca_irq",
> +
> +	.status_base = SDW_SCP_SDCA_INT1,
> +	.unmask_base = SDW_SCP_SDCA_INTMASK1,
> +	.ack_base = SDW_SCP_SDCA_INT1,
> +	.num_regs = 4,
> +
> +	.irqs = regmap_irqs,
> +	.num_irqs = SDCA_MAX_INTERRUPTS,
> +
> +	.runtime_pm = true,

can you clarify what this last initialization does? runtime_pm is far from obvious for SDCA with the different levels between the SoundWire bus (enumeration, clock stop, register access, etc) and Function management.

> +};
> +
> +static irqreturn_t base_handler(int irq, void *data)
> +{
> +	struct sdca_interrupt *interrupt = data;
> +	struct device *dev = interrupt->component->dev;
> +
> +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);

is this saying this function is really a placeholder for something else?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sdca_irq_request_locked(struct device *dev,
> +				   struct sdca_interrupt_info *info,
> +				   int sdca_irq, const char *name,
> +				   irq_handler_t handler, void *data)
> +{
> +	int irq;
> +	int ret;
> +
> +	irq = regmap_irq_get_virq(info->irq_data, sdca_irq);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, handler,
> +					IRQF_ONESHOT, name, data);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "requested irq %d for %s\n", irq, name);

might be worth adding the function and entity name?

> +
> +	return 0;
> +}
> +
> +/**
> + * sdca_request_irq - request an individual SDCA interrupt
> + * @dev: Pointer to the struct device against which things should be allocated.
> + * @interrupt_info: Pointer to the interrupt information structure.
> + * @sdca_irq: SDCA interrupt position.
> + * @name: Name to be given to the IRQ.
> + * @handler: A callback thread function to be called for the IRQ.
> + * @data: Private data pointer that will be passed to the handler.
> + *
> + * Typically this is handled internally by sdca_irq_populate, however if
> + * a device requires custom IRQ handling this can be called manually before
> + * calling sdca_irq_populate, which will then skip that IRQ whilst processing.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
> +		     int sdca_irq, const char *name, irq_handler_t handler,
> +		     void *data)
> +{
> +	int ret;
> +
> +	if (sdca_irq < 0 || sdca_irq > SDCA_MAX_INTERRUPTS) {
> +		dev_err(dev, "bad irq request: %d\n", sdca_irq);
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&info->irq_lock);
> +
> +	ret = sdca_irq_request_locked(dev, info, sdca_irq, name, handler, data);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq %s: %d\n", name, ret);
> +		return ret;
> +	}
> +
> +	info->irqs[sdca_irq].externally_requested = true;

this looks like generic/core code, not sure what's 'external' about this...

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_data_populate - Populate common interrupt data
> + * @component: Pointer to the ASoC component for the Function.
> + * @function: Pointer to the SDCA Function.
> + * @entity: Pointer to the SDCA Entity.
> + * @control: Pointer to the SDCA Control.
> + * @interrupt: Pointer to the SDCA interrupt for this IRQ.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_data_populate(struct snd_soc_component *component,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   struct sdca_control *control,
> +			   struct sdca_interrupt *interrupt)
> +{
> +	struct device *dev = component->dev;
> +	const char *name;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%s %s %s", function->desc->name,
> +			      entity->label, control->label);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	interrupt->name = name;
> +	interrupt->component = component;
> +	interrupt->function = function;
> +	interrupt->entity = entity;
> +	interrupt->control = control;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_data_populate, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_populate - Request all the individual IRQs for an SDCA Function
> + * @function: Pointer to the SDCA Function.
> + * @component: Pointer to the ASoC component for the Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + *
> + * Typically this would be called from the driver for a single SDCA Function.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_populate(struct sdca_function_data *function,
> +		      struct snd_soc_component *component,
> +		      struct sdca_interrupt_info *info)
> +{
> +	struct device *dev = component->dev;
> +	int i, j;
> +
> +	guard(mutex)(&info->irq_lock);
> +
> +	for (i = 0; i < function->num_entities; i++) {
> +		struct sdca_entity *entity = &function->entities[i];
> +
> +		for (j = 0; j < entity->num_controls; j++) {
> +			struct sdca_control *control = &entity->controls[j];
> +			int irq = control->interrupt_position;
> +			struct sdca_interrupt *interrupt;
> +			const char *name;
> +			int ret;
> +
> +			if (irq == SDCA_NO_INTERRUPT) {
> +				continue;
> +			} else if (irq < 0 || irq >= SDCA_MAX_INTERRUPTS) {
> +				dev_err(dev, "bad irq position: %d\n", irq);
> +				return -EINVAL;
> +			}
> +
> +			interrupt = &info->irqs[irq];
> +
> +			if (interrupt->externally_requested) {
> +				dev_dbg(dev,
> +					"skipping irq %d, externally requested\n",
> +					irq);
> +				continue;
> +			}
> +
> +			ret = sdca_irq_data_populate(component, function, entity,
> +						     control, interrupt);
> +			if (ret)
> +				return ret;
> +
> +			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
> +						      base_handler, interrupt);
> +			if (ret) {
> +				dev_err(dev, "failed to request irq %s: %d\n",
> +					name, ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_populate, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_allocate - allocate an SDCA interrupt structure for a device
> + * @dev: Device pointer against which things should be allocated.
> + * @regmap: regmap to be used for accessing the SDCA IRQ registers.
> + * @irq: The interrupt number.
> + *
> + * Typically this would be called from the top level driver for the whole
> + * SDCA device, as only a single instance is required across all Functions
> + * on the device.
> + *
> + * Return: A pointer to the allocated sdca_interrupt_info struct, or an
> + * error code.
> + */
> +struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
> +					      struct regmap *regmap, int irq)
> +{
> +	struct sdca_interrupt_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->irq_chip = sdca_irq_chip;
> +
> +	devm_mutex_init(dev, &info->irq_lock);
> +
> +	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
> +				       &info->irq_chip, &info->irq_data);
> +	if (ret) {
> +		dev_err(dev, "failed to register irq chip: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	dev_dbg(dev, "registered on irq %d\n", irq);

not sure how helpful this message might be, it'll be rather cryptic without additional context on which interrupt this code handles.

> +
> +	return info;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_allocate, "SND_SOC_SDCA_IRQ");
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SDCA IRQ library");

Re: [PATCH 7/7] ASoC: SDCA: Add some initial IRQ handlers

[Pierre-Louis Bossart]

> +static irqreturn_t function_status_handler(int irq, void *data)
> +{
> +	struct sdca_interrupt *interrupt = data;
> +	struct device *dev = interrupt->component->dev;
> +	unsigned int reg, val;
> +	unsigned long status;
> +	unsigned int mask;
> +	int ret;
> +
> +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> +			   interrupt->control->sel, 0);
> +
> +	ret = regmap_read(interrupt->component->regmap, reg, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read function status: %d\n", ret);
> +		return IRQ_NONE;

usually when a read fails, the entire device is in the weeds. Not sure squelching the interrupts is wise, something more drastic should happen.

> +	}
> +
> +	dev_dbg(dev, "function status: %#x\n", val);
> +
> +	status = val;
> +	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
> +		mask = 1 << mask;
> +
> +		switch (mask) {
> +		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
> +			//FIXME: Add init writes
> +			break;

between here...

> +		case SDCA_CTL_ENTITY_0_FUNCTION_FAULT:
> +			dev_err(dev, "function fault\n");
> +			break;
> +		case SDCA_CTL_ENTITY_0_UMP_SEQUENCE_FAULT:
> +			dev_err(dev, "ump sequence fault\n");
> +			break;
> +		case SDCA_CTL_ENTITY_0_DEVICE_NEWLY_ATTACHED:
> +		case SDCA_CTL_ENTITY_0_INTS_DISABLED_ABNORMALLY:
> +		case SDCA_CTL_ENTITY_0_STREAMING_STOPPED_ABNORMALLY:
> +		case SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET:

... and here, the status bit essentially mean the Function isn't working as expected. I really don't see the point of attempting to write a register below, we'd need something that essentially resets the SoundWire device to restart from a known position.

> +		case SDCA_CTL_ENTITY_0_FUNCTION_BUSY:
> +			break;

Conversely this one seems harmless, FUNCTION_BUSY only impacts specific SDCA registers and this status cannot have any bearing on how to deal with interrupts.

> +		}
> +	}
> +
> +	ret = regmap_write(interrupt->component->regmap, reg, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to clear function status: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t detected_mode_handler(int irq, void *data)
> +{
> +	struct sdca_interrupt *interrupt = data;
> +	struct snd_soc_component *component = interrupt->component;
> +	struct device *dev = component->dev;
> +	struct snd_soc_card *card = component->card;
> +	struct rw_semaphore *rwsem = &card->snd_card->controls_rwsem;
> +	struct snd_kcontrol *kctl = interrupt->priv;
> +	struct snd_ctl_elem_value ucontrol;
> +	struct soc_enum *soc_enum;
> +	unsigned int reg, val;
> +	int ret;
> +
> +	if (!kctl) {
> +		const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s %s",
> +							   interrupt->entity->label,
> +							   SDCA_CTL_SELECTED_MODE_NAME);
> +
> +		if (!name)
> +			return -ENOMEM;
> +
> +		kctl = snd_soc_component_get_kcontrol(component, name);
> +		if (!kctl) {
> +			dev_dbg(dev, "control not found: %s\n", name);
> +			return IRQ_NONE;
> +		}
> +
> +		interrupt->priv = kctl;
> +	}
> +
> +	soc_enum = (struct soc_enum *)kctl->private_value;
> +
> +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> +			   interrupt->control->sel, 0);

Humm, I have to walk back what I wrote above, if FUNCTION_BUSY is set the read below can be problematic, no?

> +	ret = regmap_read(component->regmap, reg, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read detected mode: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	switch (val) {
> +	case SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS:
> +	case SDCA_DETECTED_MODE_JACK_UNKNOWN:
> +		reg = SDW_SDCA_CTL(interrupt->function->desc->adr,
> +				   interrupt->entity->id,
> +				   SDCA_CTL_GE_SELECTED_MODE, 0);
> +
> +		/*
> +		 * Selected mode is not typically a volatile register, but
> +		 * force a read from the hardware in the case detected mode
> +		 * is unknown to see what the device selected as a "safe"
> +		 * option.
> +		 */

I am not sure this explanation is correct. I was my understanding that when there's a jack interrupt, both the DETECTED and SELECTED values are set by hardware, but SELECTED can be overridden by higher-level software or user interaction. DETECTED is RO, SELECTED is R/W IIRC.

> +		regcache_drop_region(component->regmap, reg, reg);
> +
> +		ret = regmap_read(component->regmap, reg, &val);
> +		if (ret) {
> +			dev_err(dev, "failed to re-check selected mode: %d\n", ret);
> +			return IRQ_NONE;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	dev_dbg(dev, "%s: %#x\n", interrupt->name, val);
> +
> +	ucontrol.value.enumerated.item[0] = snd_soc_enum_val_to_item(soc_enum, val);
> +
> +	down_write(rwsem);
> +	ret = kctl->put(kctl, &ucontrol);
> +	up_write(rwsem);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to update selected mode: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int sdca_irq_request_locked(struct device *dev,
>  				   struct sdca_interrupt_info *info,
>  				   int sdca_irq, const char *name,
> @@ -202,6 +339,7 @@ int sdca_irq_populate(struct sdca_function_data *function,
>  			struct sdca_control *control = &entity->controls[j];
>  			int irq = control->interrupt_position;
>  			struct sdca_interrupt *interrupt;
> +			irq_handler_t handler;
>  			const char *name;
>  			int ret;
>  
> @@ -226,8 +364,23 @@ int sdca_irq_populate(struct sdca_function_data *function,
>  			if (ret)
>  				return ret;
>  
> +			handler = base_handler;
> +
> +			switch (entity->type) {
> +			case SDCA_ENTITY_TYPE_ENTITY_0:
> +				if (control->sel == SDCA_CTL_ENTITY_0_FUNCTION_STATUS)
> +					handler = function_status_handler;
> +				break;
> +			case SDCA_ENTITY_TYPE_GE:
> +				if (control->sel == SDCA_CTL_GE_DETECTED_MODE)
> +					handler = detected_mode_handler;
> +				break;
> +			default:
> +				break;
> +			}
> +
>  			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
> -						      base_handler, interrupt);
> +						      handler, interrupt);
>  			if (ret) {
>  				dev_err(dev, "failed to request irq %s: %d\n",
>  					name, ret);

Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Charles Keepax]

On Tue, Jun 10, 2025 at 10:52:30AM +0200, Pierre-Louis Bossart wrote:
> > + * @externally_requested: Internal flag used to check if something has already
> > + * requested the interrupt.
> 
> I am not following what 'externally' and 'something'
> refer to. Each interrupt is allocated to a specific
> function/entity/control, this hints at another agent getting in
> the way but that's not really how this is supposed to work.
> 
> This deserves additional clarifications IMHO.

I can update the commit message/comment a bit, but the idea
is mostly we are creating an SDCA library here, so there are
two possible IRQ flows, the SDCA framework can handle the IRQ
internally, ie. using the handlers present in sdca_interrupts.c or
the client driver can register a handler for the IRQ directly to
do additional magic. This flag lets the core know that was done.

> > + * @irq_lock: Protects access to the list of sdca_interrupt structures.
> > + */
> > +struct sdca_interrupt_info {
> > +	struct regmap_irq_chip irq_chip;
> > +	struct regmap_irq_chip_data *irq_data;
> > +
> > +	struct sdca_interrupt irqs[SDCA_MAX_INTERRUPTS];
> > +
> > +	struct mutex irq_lock; /* Protect accesses to irqs list */
> 
> Can you elaborate on what might conflict?
> 
> The only thing I can think of in terms of required locking
> is access to the common SDCA interrupt registers, but that's
> different to the irq list.

By the nature of SDCA there is one set of IRQs for the device, so
the irqs list may be accessed from different functions, these
will by asynchronous so best to gate access to make sure
everything is happy.

> > +static const struct regmap_irq_chip sdca_irq_chip = {
> > +	.name = "sdca_irq",
> > +
> > +	.status_base = SDW_SCP_SDCA_INT1,
> > +	.unmask_base = SDW_SCP_SDCA_INTMASK1,
> > +	.ack_base = SDW_SCP_SDCA_INT1,
> > +	.num_regs = 4,
> > +
> > +	.irqs = regmap_irqs,
> > +	.num_irqs = SDCA_MAX_INTERRUPTS,
> > +
> > +	.runtime_pm = true,
> 
> can you clarify what this last initialization does? runtime_pm
> is far from obvious for SDCA with the different levels between
> the SoundWire bus (enumeration, clock stop, register access,
> etc) and Function management.

Means the regmap IRQ handler will do a PM runtime get whilst
handling the IRQ, which will ensure the both the device and the
controller are powered up, which will be necessary for
communicating with the device.

> > +static irqreturn_t base_handler(int irq, void *data)
> > +{
> > +	struct sdca_interrupt *interrupt = data;
> > +	struct device *dev = interrupt->component->dev;
> > +
> > +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
> 
> is this saying this function is really a placeholder for
> something else?

Yeah basically, this will be assigned to any IRQ that doesn't
have an implemented handler. For now that is most things, but
even in the end once the core is more flushed out this will likely
persist as IRQs are control specific in SDCA. This will always be
necessary for controls that don't have any spec mandated handling
but could get an IRQ for implementation specific purposes and
would have a custom handler registered by the client driver. It
is really a default handler to warn the user/system implementor
that some code is likely missing for a part.

> > +static int sdca_irq_request_locked(struct device *dev,
> > +				   struct sdca_interrupt_info *info,
> > +				   int sdca_irq, const char *name,
> > +				   irq_handler_t handler, void *data)
> > +{
> > +	int irq;
> > +	int ret;
> > +
> > +	irq = regmap_irq_get_virq(info->irq_data, sdca_irq);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	ret = devm_request_threaded_irq(dev, irq, NULL, handler,
> > +					IRQF_ONESHOT, name, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(dev, "requested irq %d for %s\n", irq, name);
> 
> might be worth adding the function and entity name?

That information isn't really available here, for handlers
registered by the SDCA core name will be name up of the function,
entity and control names so the information will be present. For
custom IRQ handlers it is up to the requesting driver to specify
a good name.

> > +int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
> > +		     int sdca_irq, const char *name, irq_handler_t handler,
> > +		     void *data)
> > +{
> > +	int ret;
> > +
> > +	if (sdca_irq < 0 || sdca_irq > SDCA_MAX_INTERRUPTS) {
> > +		dev_err(dev, "bad irq request: %d\n", sdca_irq);
> > +		return -EINVAL;
> > +	}
> > +
> > +	guard(mutex)(&info->irq_lock);
> > +
> > +	ret = sdca_irq_request_locked(dev, info, sdca_irq, name, handler, data);
> > +	if (ret) {
> > +		dev_err(dev, "failed to request irq %s: %d\n", name, ret);
> > +		return ret;
> > +	}
> > +
> > +	info->irqs[sdca_irq].externally_requested = true;
> 
> this looks like generic/core code, not sure what's 'external'
> about this...

The key point here is that handler is passed into the function,
that is the destinction between an internal and external here,
internal uses a built in handler, external gets handed a handler
by the client driver.

> > +/**
> > + * sdca_irq_allocate - allocate an SDCA interrupt structure for a device
> > + * @dev: Device pointer against which things should be allocated.
> > + * @regmap: regmap to be used for accessing the SDCA IRQ registers.
> > + * @irq: The interrupt number.
> > + *
> > + * Typically this would be called from the top level driver for the whole
> > + * SDCA device, as only a single instance is required across all Functions
> > + * on the device.
> > + *
> > + * Return: A pointer to the allocated sdca_interrupt_info struct, or an
> > + * error code.
> > + */
> > +struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
> > +					      struct regmap *regmap, int irq)
> > +{
> > +	struct sdca_interrupt_info *info;
> > +	int ret;
> > +
> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	info->irq_chip = sdca_irq_chip;
> > +
> > +	devm_mutex_init(dev, &info->irq_lock);
> > +
> > +	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
> > +				       &info->irq_chip, &info->irq_data);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register irq chip: %d\n", ret);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	dev_dbg(dev, "registered on irq %d\n", irq);
> 
> not sure how helpful this message might be, it'll be rather
> cryptic without additional context on which interrupt this
> code handles.

This is registering the primary IRQ it is the entry point, so
it isn't a specific IRQ in the SDCA sense. I could change it to
something like "SDCA root irq registered on %d\n"?

Thanks,
Charles

Re: [PATCH 7/7] ASoC: SDCA: Add some initial IRQ handlers

[Mark Brown]

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

On Tue, Jun 10, 2025 at 11:07:00AM +0200, Pierre-Louis Bossart wrote:

> > +static irqreturn_t function_status_handler(int irq, void *data)
> > +{

> > +	ret = regmap_read(interrupt->component->regmap, reg, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to read function status: %d\n", ret);
> > +		return IRQ_NONE;

> usually when a read fails, the entire device is in the weeds. Not sure squelching the interrupts is wise, something more drastic should happen.

It's the correct response in terms of returning from the interrupt
handler at least, it tells genirq that we weren't able to handle the
interrupt.  If it's a level triggered interrupt (or otherwise keeps
firing without being handled) then it'll eventually get shut off by the
core.  We could potentially do some Soundwire thing to try to kick
things into life, though it might be better to punt that to a higher
layer.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

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

Re: [PATCH 7/7] ASoC: SDCA: Add some initial IRQ handlers

[Charles Keepax]

On Tue, Jun 10, 2025 at 11:07:00AM +0200, Pierre-Louis Bossart wrote:
> > +	ret = regmap_read(interrupt->component->regmap, reg, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to read function status: %d\n", ret);
> > +		return IRQ_NONE;
> 
> usually when a read fails, the entire device is in the
> weeds. Not sure squelching the interrupts is wise, something
> more drastic should happen.

I am not sure I really want to get to far into complex error
handling at the moment. I think I would lean towards focusing on
getting the functional case working first. This is also pretty
much what all audio drivers do on register read errors log a
message, abort the current operation, and carry on. There isn't
great value in doing more unless one goes so far as to actually
attempt to reboot the device and recover, which I would definitely
put outside the scope of what we are trying to achieve here.

> > +	dev_dbg(dev, "function status: %#x\n", val);
> > +
> > +	status = val;
> > +	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
> > +		mask = 1 << mask;
> > +
> > +		switch (mask) {
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
> > +			//FIXME: Add init writes
> > +			break;
> 
> between here...
> 
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_FAULT:
> > +			dev_err(dev, "function fault\n");
> > +			break;
> > +		case SDCA_CTL_ENTITY_0_UMP_SEQUENCE_FAULT:
> > +			dev_err(dev, "ump sequence fault\n");
> > +			break;
> > +		case SDCA_CTL_ENTITY_0_DEVICE_NEWLY_ATTACHED:
> > +		case SDCA_CTL_ENTITY_0_INTS_DISABLED_ABNORMALLY:
> > +		case SDCA_CTL_ENTITY_0_STREAMING_STOPPED_ABNORMALLY:
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET:
> 
> ... and here, the status bit essentially mean the Function isn't
> working as expected. I really don't see the point of attempting
> to write a register below, we'd need something that essentially
> resets the SoundWire device to restart from a known position.

The FAULT ones definitely, hence why I log an error. The others
its less clear, definitely on first boot both NEWLY_ATTACHED
and RESET are likely going to be set. The two ABNORMALLY's are
also a bit sketchy, if you look in Table 198 these can be set
after any reset. Basically where one gets to is  all of these
can basically happen during normal operation.

One could probably get into more complex trying to filter out
post reset events and catch later ones, but again I am hesitant
to get to far into complex error fielding like that. It seems
like it would make more sense to tackle those problems once we
have some situations where people are actually hitting problems
then the requirements might start to become a little more clear.

At most right now I would probably go for a debug in here, but it
feels redundant as there is the line above logging the function
status value. Also if a device wants to respond to these with
more strenuous measures it could override the function status
IRQ and taking device specific action.

> > +		case SDCA_CTL_ENTITY_0_FUNCTION_BUSY:
> > +			break;
> 
> Conversely this one seems harmless, FUNCTION_BUSY only impacts
> specific SDCA registers and this status cannot have any bearing
> on how to deal with interrupts.

Yeah this should be pretty harmless... well unless its a device
that function busy's for random reasons (ie. reasons other than a
deferred register operation), but that is not something we are
currently adding support for.

> > +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> > +			   interrupt->control->sel, 0);
> 
> Humm, I have to walk back what I wrote above, if FUNCTION_BUSY
> is set the read below can be problematic, no?

In the current register operation function busy model, this will
always be fine as this operation will block on the regmap lock
which should be held by the previous register operation that is
causing the function busy.

> > +	switch (val) {
> > +	case SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS:
> > +	case SDCA_DETECTED_MODE_JACK_UNKNOWN:
> > +		reg = SDW_SDCA_CTL(interrupt->function->desc->adr,
> > +				   interrupt->entity->id,
> > +				   SDCA_CTL_GE_SELECTED_MODE, 0);
> > +
> > +		/*
> > +		 * Selected mode is not typically a volatile register, but
> > +		 * force a read from the hardware in the case detected mode
> > +		 * is unknown to see what the device selected as a "safe"
> > +		 * option.
> > +		 */
> 
> I am not sure this explanation is correct. I was my
> understanding that when there's a jack interrupt, both
> the DETECTED and SELECTED values are set by hardware, but
> SELECTED can be overridden by higher-level software or user
> interaction. DETECTED is RO, SELECTED is R/W IIRC.

That is basically what I am trying to say in the comment. Normal
proceedure from the class side is to read the DETECTED and write
that into the SELECTED, even though the hardware will have likely
already done that. But it is slightly unusual to have a register
that is Class Access and RW but still written by the device. RW
registers are usually cached, here we need to drop that cache to
ensure we read the value which the firmware has tampered with,
because we if DETECTED was showing unknown we want to know what
"safe" value the firmware picked for SELECTED rather than our
cached value.

Thanks,
Charles

Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Pierre-Louis Bossart]

On 6/10/25 12:21, Charles Keepax wrote:
> On Tue, Jun 10, 2025 at 10:52:30AM +0200, Pierre-Louis Bossart wrote:
>>> + * @externally_requested: Internal flag used to check if something has already
>>> + * requested the interrupt.
>>
>> I am not following what 'externally' and 'something'
>> refer to. Each interrupt is allocated to a specific
>> function/entity/control, this hints at another agent getting in
>> the way but that's not really how this is supposed to work.
>>
>> This deserves additional clarifications IMHO.
> 
> I can update the commit message/comment a bit, but the idea
> is mostly we are creating an SDCA library here, so there are
> two possible IRQ flows, the SDCA framework can handle the IRQ
> internally, ie. using the handlers present in sdca_interrupts.c or
> the client driver can register a handler for the IRQ directly to
> do additional magic. This flag lets the core know that was done.

I would have expected a base behavior to deal with the interrupt itself, and optionally a vendor-specific extension for additional magic. I don't really get the notion of 'two possible flows'.

>>> + * @irq_lock: Protects access to the list of sdca_interrupt structures.
>>> + */
>>> +struct sdca_interrupt_info {
>>> +	struct regmap_irq_chip irq_chip;
>>> +	struct regmap_irq_chip_data *irq_data;
>>> +
>>> +	struct sdca_interrupt irqs[SDCA_MAX_INTERRUPTS];
>>> +
>>> +	struct mutex irq_lock; /* Protect accesses to irqs list */
>>
>> Can you elaborate on what might conflict?
>>
>> The only thing I can think of in terms of required locking
>> is access to the common SDCA interrupt registers, but that's
>> different to the irq list.
> 
> By the nature of SDCA there is one set of IRQs for the device, so
> the irqs list may be accessed from different functions, these
> will by asynchronous so best to gate access to make sure
> everything is happy.

right so it's really protection between concurrent accesses to the irq list initiated by separate function drivers.

>>> +static const struct regmap_irq_chip sdca_irq_chip = {
>>> +	.name = "sdca_irq",
>>> +
>>> +	.status_base = SDW_SCP_SDCA_INT1,
>>> +	.unmask_base = SDW_SCP_SDCA_INTMASK1,
>>> +	.ack_base = SDW_SCP_SDCA_INT1,
>>> +	.num_regs = 4,
>>> +
>>> +	.irqs = regmap_irqs,
>>> +	.num_irqs = SDCA_MAX_INTERRUPTS,
>>> +
>>> +	.runtime_pm = true,
>>
>> can you clarify what this last initialization does? runtime_pm
>> is far from obvious for SDCA with the different levels between
>> the SoundWire bus (enumeration, clock stop, register access,
>> etc) and Function management.
> 
> Means the regmap IRQ handler will do a PM runtime get whilst
> handling the IRQ, which will ensure the both the device and the
> controller are powered up, which will be necessary for
> communicating with the device.

this is kind of odd, the device needs to already be up before you have an SDCA interrupt, no?
The only case where the device and controller would be in a low-power state would be in the clock-stop, but that's different to the SDCA interrupt, it's the SoundWire 'keep data line high' mechanism.

>>> +static irqreturn_t base_handler(int irq, void *data)
>>> +{
>>> +	struct sdca_interrupt *interrupt = data;
>>> +	struct device *dev = interrupt->component->dev;
>>> +
>>> +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
>>
>> is this saying this function is really a placeholder for
>> something else?
> 
> Yeah basically, this will be assigned to any IRQ that doesn't
> have an implemented handler. For now that is most things, but
> even in the end once the core is more flushed out this will likely
> persist as IRQs are control specific in SDCA. This will always be
> necessary for controls that don't have any spec mandated handling
> but could get an IRQ for implementation specific purposes and
> would have a custom handler registered by the client driver. It
> is really a default handler to warn the user/system implementor
> that some code is likely missing for a part.

ok, not sure a dev_warn is required though?

>>> +static int sdca_irq_request_locked(struct device *dev,
>>> +				   struct sdca_interrupt_info *info,
>>> +				   int sdca_irq, const char *name,
>>> +				   irq_handler_t handler, void *data)
>>> +{
>>> +	int irq;
>>> +	int ret;
>>> +
>>> +	irq = regmap_irq_get_virq(info->irq_data, sdca_irq);
>>> +	if (irq < 0)
>>> +		return irq;
>>> +
>>> +	ret = devm_request_threaded_irq(dev, irq, NULL, handler,
>>> +					IRQF_ONESHOT, name, data);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	dev_dbg(dev, "requested irq %d for %s\n", irq, name);
>>
>> might be worth adding the function and entity name?
> 
> That information isn't really available here, for handlers
> registered by the SDCA core name will be name up of the function,
> entity and control names so the information will be present. For
> custom IRQ handlers it is up to the requesting driver to specify
> a good name.
> 
>>> +int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
>>> +		     int sdca_irq, const char *name, irq_handler_t handler,
>>> +		     void *data)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (sdca_irq < 0 || sdca_irq > SDCA_MAX_INTERRUPTS) {
>>> +		dev_err(dev, "bad irq request: %d\n", sdca_irq);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	guard(mutex)(&info->irq_lock);
>>> +
>>> +	ret = sdca_irq_request_locked(dev, info, sdca_irq, name, handler, data);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to request irq %s: %d\n", name, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	info->irqs[sdca_irq].externally_requested = true;
>>
>> this looks like generic/core code, not sure what's 'external'
>> about this...
> 
> The key point here is that handler is passed into the function,
> that is the destinction between an internal and external here,
> internal uses a built in handler, external gets handed a handler
> by the client driver.

I am not sure I follow this one, you could have a common mechanism followed by a vendor-specific one. Why would there be a choice to make? IIRC the mechanism I implemented in the early stages didn't have this architectural notion of internal/external, just a vendor-specific callback once the interrupt was cleared.

> 
>>> +/**
>>> + * sdca_irq_allocate - allocate an SDCA interrupt structure for a device
>>> + * @dev: Device pointer against which things should be allocated.
>>> + * @regmap: regmap to be used for accessing the SDCA IRQ registers.
>>> + * @irq: The interrupt number.
>>> + *
>>> + * Typically this would be called from the top level driver for the whole
>>> + * SDCA device, as only a single instance is required across all Functions
>>> + * on the device.
>>> + *
>>> + * Return: A pointer to the allocated sdca_interrupt_info struct, or an
>>> + * error code.
>>> + */
>>> +struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
>>> +					      struct regmap *regmap, int irq)
>>> +{
>>> +	struct sdca_interrupt_info *info;
>>> +	int ret;
>>> +
>>> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>>> +	if (!info)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	info->irq_chip = sdca_irq_chip;
>>> +
>>> +	devm_mutex_init(dev, &info->irq_lock);
>>> +
>>> +	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
>>> +				       &info->irq_chip, &info->irq_data);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to register irq chip: %d\n", ret);
>>> +		return ERR_PTR(ret);
>>> +	}
>>> +
>>> +	dev_dbg(dev, "registered on irq %d\n", irq);
>>
>> not sure how helpful this message might be, it'll be rather
>> cryptic without additional context on which interrupt this
>> code handles.
> 
> This is registering the primary IRQ it is the entry point, so
> it isn't a specific IRQ in the SDCA sense. I could change it to
> something like "SDCA root irq registered on %d\n"?

ok

Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Charles Keepax]

On Tue, Jun 10, 2025 at 07:55:23PM +0200, Pierre-Louis Bossart wrote:
> On 6/10/25 12:21, Charles Keepax wrote:
> > On Tue, Jun 10, 2025 at 10:52:30AM +0200, Pierre-Louis Bossart wrote:

Apologies for the delay, I was sure I responded to this but I
must have done one of those writing the email then failing to
send it tricks.

> >>> + * @externally_requested: Internal flag used to check if something has already
> >>> + * requested the interrupt.
> >>
> >> I am not following what 'externally' and 'something'
> >> refer to. Each interrupt is allocated to a specific
> >> function/entity/control, this hints at another agent getting in
> >> the way but that's not really how this is supposed to work.
> >>
> >> This deserves additional clarifications IMHO.
> > 
> > I can update the commit message/comment a bit, but the idea
> > is mostly we are creating an SDCA library here, so there are
> > two possible IRQ flows, the SDCA framework can handle the IRQ
> > internally, ie. using the handlers present in sdca_interrupts.c or
> > the client driver can register a handler for the IRQ directly to
> > do additional magic. This flag lets the core know that was done.
> 
> I would have expected a base behavior to deal with the interrupt
> itself, and optionally a vendor-specific extension for additional
> magic. I don't really get the notion of 'two possible flows'.

There really isn't a lot of difference between the two:

  1) Internal: the handler is all class compliant the core just
     registers it for you:
      - device driver calls sdca_irq_allocate to register the
        root IRQ.
      - function driver calls sdca_irq_populate to register all
        the handlers.
  2) External: the handler is not class compliant the end driver
     should register its own handler to do whatever magic:
      - device driver calls sdca_irq_allocate to register the
        root IRQ.
      - function driver calls sdca_irq_request for the IRQs that
        need custom handling.
      - function driver calls sdca_irq_populate to register any
        remaining handlers, this can be skipped if all IRQs were
        registered through sdca_irq_request.

Its really just a cutting down on boiler plate code thing, one
could always make the client driver register all IRQs through
sdca_irq_request and then you wouldn't need the external flag,
but its nice for compliant devices if you can just have a single
register all IRQs call.

> >>> +	.runtime_pm = true,
> >>
> >> can you clarify what this last initialization does? runtime_pm
> >> is far from obvious for SDCA with the different levels between
> >> the SoundWire bus (enumeration, clock stop, register access,
> >> etc) and Function management.
> > 
> > Means the regmap IRQ handler will do a PM runtime get whilst
> > handling the IRQ, which will ensure the both the device and the
> > controller are powered up, which will be necessary for
> > communicating with the device.
> 
> this is kind of odd, the device needs to already be up before
> you have an SDCA interrupt, no?
> The only case where the device and controller would be in a
> low-power state would be in the clock-stop, but that's different
> to the SDCA interrupt, it's the SoundWire 'keep data line high'
> mechanism.

I mean yes the device is probably already up when the IRQ is
first processed by the SoundWire. In general its just good
practice to be holding a runtime reference whilst interacting
with the part, stop things turning off whilst you do so.

> >>> +static irqreturn_t base_handler(int irq, void *data)
> >>> +{
> >>> +	struct sdca_interrupt *interrupt = data;
> >>> +	struct device *dev = interrupt->component->dev;
> >>> +
> >>> +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
> >>
> >> is this saying this function is really a placeholder for
> >> something else?
> > 
> > Yeah basically, this will be assigned to any IRQ that doesn't
> > have an implemented handler. For now that is most things, but
> > even in the end once the core is more flushed out this will likely
> > persist as IRQs are control specific in SDCA. This will always be
> > necessary for controls that don't have any spec mandated handling
> > but could get an IRQ for implementation specific purposes and
> > would have a custom handler registered by the client driver. It
> > is really a default handler to warn the user/system implementor
> > that some code is likely missing for a part.
> 
> ok, not sure a dev_warn is required though?

It is useful information to the user, it typically indicates the
part is doing something that has no software support. I would be
happy to downgrade to an info or dbg if you felt strongly, but
personally I think a warn feels right here.

> >>> +	info->irqs[sdca_irq].externally_requested = true;
> >>
> >> this looks like generic/core code, not sure what's 'external'
> >> about this...
> > 
> > The key point here is that handler is passed into the function,
> > that is the destinction between an internal and external here,
> > internal uses a built in handler, external gets handed a handler
> > by the client driver.
> 
> I am not sure I follow this one, you could have a common
> mechanism followed by a vendor-specific one. Why would there be
> a choice to make? IIRC the mechanism I implemented in the early
> stages didn't have this architectural notion of internal/external,
> just a vendor-specific callback once the interrupt was cleared.
> 

The regmap IRQ always clears the interrupt, so that is the same
as your previous implementation. The handler registered here is
analogous to your vendor-specific callback whether it is
internal or external. The extra layer here is just a convience,
the core can register all the IRQ handlers for you if you don't
need any non-class compliant handling. Just saves a bunch of
boiler plate in the drivers registering all the IRQs.

Thanks,
Charles

Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Pierre-Louis Bossart]

On 6/17/25 11:30, Charles Keepax wrote:
> On Tue, Jun 10, 2025 at 07:55:23PM +0200, Pierre-Louis Bossart wrote:
>> On 6/10/25 12:21, Charles Keepax wrote:
>>> On Tue, Jun 10, 2025 at 10:52:30AM +0200, Pierre-Louis Bossart wrote:
> 
> Apologies for the delay, I was sure I responded to this but I
> must have done one of those writing the email then failing to
> send it tricks.
> 
>>>>> + * @externally_requested: Internal flag used to check if something has already
>>>>> + * requested the interrupt.
>>>>
>>>> I am not following what 'externally' and 'something'
>>>> refer to. Each interrupt is allocated to a specific
>>>> function/entity/control, this hints at another agent getting in
>>>> the way but that's not really how this is supposed to work.
>>>>
>>>> This deserves additional clarifications IMHO.
>>>
>>> I can update the commit message/comment a bit, but the idea
>>> is mostly we are creating an SDCA library here, so there are
>>> two possible IRQ flows, the SDCA framework can handle the IRQ
>>> internally, ie. using the handlers present in sdca_interrupts.c or
>>> the client driver can register a handler for the IRQ directly to
>>> do additional magic. This flag lets the core know that was done.
>>
>> I would have expected a base behavior to deal with the interrupt
>> itself, and optionally a vendor-specific extension for additional
>> magic. I don't really get the notion of 'two possible flows'.
> 
> There really isn't a lot of difference between the two:
> 
>   1) Internal: the handler is all class compliant the core just
>      registers it for you:
>       - device driver calls sdca_irq_allocate to register the
>         root IRQ.
>       - function driver calls sdca_irq_populate to register all
>         the handlers.
>   2) External: the handler is not class compliant the end driver
>      should register its own handler to do whatever magic:
>       - device driver calls sdca_irq_allocate to register the
>         root IRQ.
>       - function driver calls sdca_irq_request for the IRQs that
>         need custom handling.
>       - function driver calls sdca_irq_populate to register any
>         remaining handlers, this can be skipped if all IRQs were
>         registered through sdca_irq_request.
> 
> Its really just a cutting down on boiler plate code thing, one
> could always make the client driver register all IRQs through
> sdca_irq_request and then you wouldn't need the external flag,
> but its nice for compliant devices if you can just have a single
> register all IRQs call.

ok, I see what you are trying to do and I'm fine with it. The use of the word "compliant" is somewhat problematic though, MIPI does not define nor endorse any compliance program and instead talks about 'conformance'. In the case of SDCA, a device could perfectly follow all guidelines of the spec, but require extra magic with a vendor-specific register and/or a vendor-specific extension. That's where you would need dedicated handlers.

> 
>>>>> +	.runtime_pm = true,
>>>>
>>>> can you clarify what this last initialization does? runtime_pm
>>>> is far from obvious for SDCA with the different levels between
>>>> the SoundWire bus (enumeration, clock stop, register access,
>>>> etc) and Function management.
>>>
>>> Means the regmap IRQ handler will do a PM runtime get whilst
>>> handling the IRQ, which will ensure the both the device and the
>>> controller are powered up, which will be necessary for
>>> communicating with the device.
>>
>> this is kind of odd, the device needs to already be up before
>> you have an SDCA interrupt, no?
>> The only case where the device and controller would be in a
>> low-power state would be in the clock-stop, but that's different
>> to the SDCA interrupt, it's the SoundWire 'keep data line high'
>> mechanism.
> 
> I mean yes the device is probably already up when the IRQ is
> first processed by the SoundWire. In general its just good
> practice to be holding a runtime reference whilst interacting
> with the part, stop things turning off whilst you do so.

ok.

>>>>> +static irqreturn_t base_handler(int irq, void *data)
>>>>> +{
>>>>> +	struct sdca_interrupt *interrupt = data;
>>>>> +	struct device *dev = interrupt->component->dev;
>>>>> +
>>>>> +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
>>>>
>>>> is this saying this function is really a placeholder for
>>>> something else?
>>>
>>> Yeah basically, this will be assigned to any IRQ that doesn't
>>> have an implemented handler. For now that is most things, but
>>> even in the end once the core is more flushed out this will likely
>>> persist as IRQs are control specific in SDCA. This will always be
>>> necessary for controls that don't have any spec mandated handling
>>> but could get an IRQ for implementation specific purposes and
>>> would have a custom handler registered by the client driver. It
>>> is really a default handler to warn the user/system implementor
>>> that some code is likely missing for a part.
>>
>> ok, not sure a dev_warn is required though?
> 
> It is useful information to the user, it typically indicates the
> part is doing something that has no software support. I would be
> happy to downgrade to an info or dbg if you felt strongly, but
> personally I think a warn feels right here.

it's ok for now, once users complain about misleading messages we can always scale this back.

>>>>> +	info->irqs[sdca_irq].externally_requested = true;
>>>>
>>>> this looks like generic/core code, not sure what's 'external'
>>>> about this...
>>>
>>> The key point here is that handler is passed into the function,
>>> that is the destinction between an internal and external here,
>>> internal uses a built in handler, external gets handed a handler
>>> by the client driver.
>>
>> I am not sure I follow this one, you could have a common
>> mechanism followed by a vendor-specific one. Why would there be
>> a choice to make? IIRC the mechanism I implemented in the early
>> stages didn't have this architectural notion of internal/external,
>> just a vendor-specific callback once the interrupt was cleared.
>>
> 
> The regmap IRQ always clears the interrupt, so that is the same
> as your previous implementation. The handler registered here is
> analogous to your vendor-specific callback whether it is
> internal or external. The extra layer here is just a convience,
> the core can register all the IRQ handlers for you if you don't
> need any non-class compliant handling. Just saves a bunch of
> boiler plate in the drivers registering all the IRQs.

ok