* [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-12-09 12:00 ` Pierre-Louis Bossart
2025-11-25 15:21 ` [PATCH 2/7] ASoC: SDCA: Add ability to connect SDCA jacks to ASoC jacks Charles Keepax
` (5 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
The jack code is perhaps a bit large for being in the interrupt
code directly. Improve the encapsulation by factoring out the
jack handling code into a new c file, as is already done for HID
and FDL. Whilst doing so also add a jack_state structure to hold
the jack state for improved expandability in the future.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
include/sound/sdca_jack.h | 27 ++++++
sound/soc/sdca/Makefile | 2 +-
sound/soc/sdca/sdca_interrupts.c | 83 ++----------------
sound/soc/sdca/sdca_jack.c | 140 +++++++++++++++++++++++++++++++
4 files changed, 175 insertions(+), 77 deletions(-)
create mode 100644 include/sound/sdca_jack.h
create mode 100644 sound/soc/sdca/sdca_jack.c
diff --git a/include/sound/sdca_jack.h b/include/sound/sdca_jack.h
new file mode 100644
index 0000000000000..9fad5f22cbb9e
--- /dev/null
+++ b/include/sound/sdca_jack.h
@@ -0,0 +1,27 @@
+/* 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_JACK_H__
+#define __SDCA_JACK_H__
+
+struct sdca_interrupt;
+struct snd_kcontrol;
+
+/**
+ * struct jack_state - Jack state structure to keep data between interrupts
+ * @kctl: Pointer to the ALSA control attached to this jack
+ */
+struct jack_state {
+ struct snd_kcontrol *kctl;
+};
+
+int sdca_jack_alloc_state(struct sdca_interrupt *interrupt);
+int sdca_jack_process(struct sdca_interrupt *interrupt);
+
+#endif // __SDCA_JACK_H__
diff --git a/sound/soc/sdca/Makefile b/sound/soc/sdca/Makefile
index f6b73275d9649..b3b0f5d94c8de 100644
--- a/sound/soc/sdca/Makefile
+++ b/sound/soc/sdca/Makefile
@@ -3,7 +3,7 @@
snd-soc-sdca-y := sdca_functions.o sdca_device.o sdca_function_device.o \
sdca_regmap.o sdca_asoc.o sdca_ump.o
snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_HID) += sdca_hid.o
-snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_IRQ) += sdca_interrupts.o
+snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_IRQ) += sdca_interrupts.o sdca_jack.o
snd-soc-sdca-$(CONFIG_SND_SOC_SDCA_FDL) += sdca_fdl.o
snd-soc-sdca-class-y := sdca_class.o
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 8f6a2adfb6fbe..ff3a7e405fdcb 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -22,6 +22,7 @@
#include <sound/sdca_function.h>
#include <sound/sdca_hid.h>
#include <sound/sdca_interrupts.h>
+#include <sound/sdca_jack.h>
#include <sound/sdca_ump.h>
#include <sound/soc-component.h>
#include <sound/soc.h>
@@ -155,14 +156,7 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
{
struct sdca_interrupt *interrupt = data;
struct device *dev = interrupt->dev;
- struct snd_soc_component *component = interrupt->component;
- struct 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 __free(kfree) = NULL;
- struct soc_enum *soc_enum;
irqreturn_t irqret = IRQ_NONE;
- unsigned int reg, val;
int ret;
ret = pm_runtime_get_sync(dev);
@@ -171,76 +165,9 @@ static irqreturn_t detected_mode_handler(int irq, void *data)
goto error;
}
- if (!kctl) {
- const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s %s",
- interrupt->entity->label,
- SDCA_CTL_SELECTED_MODE_NAME);
-
- if (!name)
- goto error;
-
- kctl = snd_soc_component_get_kcontrol(component, name);
- if (!kctl) {
- dev_dbg(dev, "control not found: %s\n", name);
- goto error;
- }
-
- 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(interrupt->function_regmap, reg, &val);
- if (ret < 0) {
- dev_err(dev, "failed to read detected mode: %d\n", ret);
- goto error;
- }
-
- 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 normally marked as volatile register
- * (RW), but here force a read from the hardware. If the
- * detected mode is unknown we need to see what the device
- * selected as a "safe" option.
- */
- regcache_drop_region(interrupt->function_regmap, reg, reg);
-
- ret = regmap_read(interrupt->function_regmap, reg, &val);
- if (ret) {
- dev_err(dev, "failed to re-check selected mode: %d\n", ret);
- goto error;
- }
- break;
- default:
- break;
- }
-
- dev_dbg(dev, "%s: %#x\n", interrupt->name, val);
-
- ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL);
- if (!ucontrol)
- goto error;
-
- 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);
+ ret = sdca_jack_process(interrupt);
+ if (ret)
goto error;
- }
-
- snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
irqret = IRQ_HANDLED;
error:
@@ -536,6 +463,10 @@ int sdca_irq_populate(struct sdca_function_data *function,
handler = function_status_handler;
break;
case SDCA_CTL_TYPE_S(GE, DETECTED_MODE):
+ ret = sdca_jack_alloc_state(interrupt);
+ if (ret)
+ return ret;
+
handler = detected_mode_handler;
break;
case SDCA_CTL_TYPE_S(XU, FDL_CURRENTOWNER):
diff --git a/sound/soc/sdca/sdca_jack.c b/sound/soc/sdca/sdca_jack.c
new file mode 100644
index 0000000000000..83b2b9cc81f00
--- /dev/null
+++ b/sound/soc/sdca/sdca_jack.c
@@ -0,0 +1,140 @@
+// 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/cleanup.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/soundwire/sdw.h>
+#include <linux/soundwire/sdw_registers.h>
+#include <linux/sprintf.h>
+#include <linux/regmap.h>
+#include <linux/rwsem.h>
+#include <sound/asound.h>
+#include <sound/control.h>
+#include <sound/sdca.h>
+#include <sound/sdca_function.h>
+#include <sound/sdca_interrupts.h>
+#include <sound/sdca_jack.h>
+#include <sound/soc-component.h>
+#include <sound/soc.h>
+
+/**
+ * sdca_jack_process - Process an SDCA jack event
+ * @interrupt: SDCA interrupt structure
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_jack_process(struct sdca_interrupt *interrupt)
+{
+ struct device *dev = interrupt->dev;
+ struct snd_soc_component *component = interrupt->component;
+ struct snd_soc_card *card = component->card;
+ struct rw_semaphore *rwsem = &card->snd_card->controls_rwsem;
+ struct jack_state *state = interrupt->priv;
+ struct snd_kcontrol *kctl = state->kctl;
+ struct snd_ctl_elem_value *ucontrol __free(kfree) = NULL;
+ 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 -ENOENT;
+ }
+
+ state->kctl = 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(interrupt->function_regmap, reg, &val);
+ if (ret < 0) {
+ dev_err(dev, "failed to read detected mode: %d\n", ret);
+ return ret;
+ }
+
+ 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 normally marked as volatile register
+ * (RW), but here force a read from the hardware. If the
+ * detected mode is unknown we need to see what the device
+ * selected as a "safe" option.
+ */
+ regcache_drop_region(interrupt->function_regmap, reg, reg);
+
+ ret = regmap_read(interrupt->function_regmap, reg, &val);
+ if (ret) {
+ dev_err(dev, "failed to re-check selected mode: %d\n", ret);
+ return ret;
+ }
+ break;
+ default:
+ break;
+ }
+
+ dev_dbg(dev, "%s: %#x\n", interrupt->name, val);
+
+ ucontrol = kzalloc(sizeof(*ucontrol), GFP_KERNEL);
+ if (!ucontrol)
+ return -ENOMEM;
+
+ 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 ret;
+ }
+
+ snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_jack_process, "SND_SOC_SDCA");
+
+/**
+ * sdca_jack_alloc_state - allocate state for a jack interrupt
+ * @interrupt: SDCA interrupt structure.
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_jack_alloc_state(struct sdca_interrupt *interrupt)
+{
+ struct device *dev = interrupt->dev;
+ struct jack_state *jack_state;
+
+ jack_state = devm_kzalloc(dev, sizeof(*jack_state), GFP_KERNEL);
+ if (!jack_state)
+ return -ENOMEM;
+
+ interrupt->priv = jack_state;
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_jack_alloc_state, "SND_SOC_SDCA");
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file
2025-11-25 15:21 ` [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file Charles Keepax
@ 2025-12-09 12:00 ` Pierre-Louis Bossart
2025-12-10 16:31 ` Charles Keepax
0 siblings, 1 reply; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-09 12:00 UTC (permalink / raw)
To: Charles Keepax, broonie
Cc: yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi, shumingf,
linux-sound, patches
> + 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 normally marked as volatile register
> + * (RW), but here force a read from the hardware. If the
> + * detected mode is unknown we need to see what the device
> + * selected as a "safe" option.
> + */
> + regcache_drop_region(interrupt->function_regmap, reg, reg);
> +
> + ret = regmap_read(interrupt->function_regmap, reg, &val);
> + if (ret) {
> + dev_err(dev, "failed to re-check selected mode: %d\n", ret);
> + return ret;
> + }
The detected_mode is reported by hardware and copied into the selected_mode, but that's not the end of it.
We should capture with at least a comment that the intent of the SDCA design was to let the user or additional vendor-specific override the selected mode. If/when we have to deal with the complicated multi-jack topologies it'll be required.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file
2025-12-09 12:00 ` Pierre-Louis Bossart
@ 2025-12-10 16:31 ` Charles Keepax
2025-12-20 11:22 ` Pierre-Louis Bossart
0 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-12-10 16:31 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On Tue, Dec 09, 2025 at 12:00:24PM +0000, Pierre-Louis Bossart wrote:
> > + 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 normally marked as volatile register
> > + * (RW), but here force a read from the hardware. If the
> > + * detected mode is unknown we need to see what the device
> > + * selected as a "safe" option.
> > + */
> > + regcache_drop_region(interrupt->function_regmap, reg, reg);
> > +
> > + ret = regmap_read(interrupt->function_regmap, reg, &val);
> > + if (ret) {
> > + dev_err(dev, "failed to re-check selected mode: %d\n", ret);
> > + return ret;
> > + }
>
> The detected_mode is reported by hardware and copied into the
> selected_mode, but that's not the end of it.
> We should capture with at least a comment that the intent of
> the SDCA design was to let the user or additional vendor-specific
> override the selected mode. If/when we have to deal with the
> complicated multi-jack topologies it'll be required.
So the theory is we are already supporting this. The jack handler
here will report the "safe" accessory type returned by the device.
But the detected mode and selected mode are exposed as ALSA
controls to user-space, so if the user wants to adjust the
accessory type they can just set the selected mode control. If
vendors want to do in driver vendor magic they will need to
register their own IRQ handler for the detected mode control, but
we provide for that through the sdca_irq_request() API.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file
2025-12-10 16:31 ` Charles Keepax
@ 2025-12-20 11:22 ` Pierre-Louis Bossart
0 siblings, 0 replies; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-20 11:22 UTC (permalink / raw)
To: Charles Keepax
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On 12/10/25 17:31, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:00:24PM +0000, Pierre-Louis Bossart wrote:
>>> + 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 normally marked as volatile register
>>> + * (RW), but here force a read from the hardware. If the
>>> + * detected mode is unknown we need to see what the device
>>> + * selected as a "safe" option.
>>> + */
>>> + regcache_drop_region(interrupt->function_regmap, reg, reg);
>>> +
>>> + ret = regmap_read(interrupt->function_regmap, reg, &val);
>>> + if (ret) {
>>> + dev_err(dev, "failed to re-check selected mode: %d\n", ret);
>>> + return ret;
>>> + }
>>
>> The detected_mode is reported by hardware and copied into the
>> selected_mode, but that's not the end of it.
>> We should capture with at least a comment that the intent of
>> the SDCA design was to let the user or additional vendor-specific
>> override the selected mode. If/when we have to deal with the
>> complicated multi-jack topologies it'll be required.
>
> So the theory is we are already supporting this. The jack handler
> here will report the "safe" accessory type returned by the device.
> But the detected mode and selected mode are exposed as ALSA
> controls to user-space, so if the user wants to adjust the
> accessory type they can just set the selected mode control. If
> vendors want to do in driver vendor magic they will need to
> register their own IRQ handler for the detected mode control, but
> we provide for that through the sdca_irq_request() API.
Sounds good, thanks for the clarification.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/7] ASoC: SDCA: Add ability to connect SDCA jacks to ASoC jacks
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
2025-11-25 15:21 ` [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-11-25 15:21 ` [PATCH 3/7] ASoC: SDCA: Add ASoC jack hookup in class driver Charles Keepax
` (4 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
Add handling for the ASoC jack API to SDCA to allow user-space to be
hooked up normally.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
include/sound/sdca_jack.h | 5 ++
sound/soc/sdca/sdca_jack.c | 106 ++++++++++++++++++++++++++++++++++++-
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/include/sound/sdca_jack.h b/include/sound/sdca_jack.h
index 9fad5f22cbb9e..3ec22046d3ebc 100644
--- a/include/sound/sdca_jack.h
+++ b/include/sound/sdca_jack.h
@@ -12,16 +12,21 @@
struct sdca_interrupt;
struct snd_kcontrol;
+struct snd_soc_jack;
/**
* struct jack_state - Jack state structure to keep data between interrupts
* @kctl: Pointer to the ALSA control attached to this jack
+ * @jack: Pointer to the ASoC jack struct for this jack
*/
struct jack_state {
struct snd_kcontrol *kctl;
+ struct snd_soc_jack *jack;
};
int sdca_jack_alloc_state(struct sdca_interrupt *interrupt);
int sdca_jack_process(struct sdca_interrupt *interrupt);
+int sdca_jack_set_jack(struct sdca_interrupt_info *info, struct snd_soc_jack *jack);
+int sdca_jack_report(struct sdca_interrupt *interrupt);
#endif // __SDCA_JACK_H__
diff --git a/sound/soc/sdca/sdca_jack.c b/sound/soc/sdca/sdca_jack.c
index 83b2b9cc81f00..5b9cf69cbcd6b 100644
--- a/sound/soc/sdca/sdca_jack.c
+++ b/sound/soc/sdca/sdca_jack.c
@@ -17,11 +17,13 @@
#include <linux/rwsem.h>
#include <sound/asound.h>
#include <sound/control.h>
+#include <sound/jack.h>
#include <sound/sdca.h>
#include <sound/sdca_function.h>
#include <sound/sdca_interrupts.h>
#include <sound/sdca_jack.h>
#include <sound/soc-component.h>
+#include <sound/soc-jack.h>
#include <sound/soc.h>
/**
@@ -114,7 +116,7 @@ int sdca_jack_process(struct sdca_interrupt *interrupt)
snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
- return 0;
+ return sdca_jack_report(interrupt);
}
EXPORT_SYMBOL_NS_GPL(sdca_jack_process, "SND_SOC_SDCA");
@@ -138,3 +140,105 @@ int sdca_jack_alloc_state(struct sdca_interrupt *interrupt)
return 0;
}
EXPORT_SYMBOL_NS_GPL(sdca_jack_alloc_state, "SND_SOC_SDCA");
+
+/**
+ * sdca_jack_set_jack - attach an ASoC jack to SDCA
+ * @info: SDCA interrupt information.
+ * @jack: ASoC jack to be attached.
+ *
+ * Return: Zero on success or a negative error code.
+ */
+int sdca_jack_set_jack(struct sdca_interrupt_info *info, struct snd_soc_jack *jack)
+{
+ int i, ret;
+
+ guard(mutex)(&info->irq_lock);
+
+ for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
+ struct sdca_interrupt *interrupt = &info->irqs[i];
+ struct sdca_control *control = interrupt->control;
+ struct sdca_entity *entity = interrupt->entity;
+ struct jack_state *jack_state;
+
+ if (!interrupt->irq)
+ continue;
+
+ switch (SDCA_CTL_TYPE(entity->type, control->sel)) {
+ case SDCA_CTL_TYPE_S(GE, DETECTED_MODE):
+ jack_state = interrupt->priv;
+ jack_state->jack = jack;
+
+ /* Report initial state in case IRQ was already handled */
+ ret = sdca_jack_report(interrupt);
+ if (ret)
+ return ret;
+ break;
+ default:
+ break;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_jack_set_jack, "SND_SOC_SDCA");
+
+int sdca_jack_report(struct sdca_interrupt *interrupt)
+{
+ struct jack_state *jack_state = interrupt->priv;
+ struct sdca_control_range *range;
+ enum sdca_terminal_type type;
+ unsigned int report = 0;
+ unsigned int reg, val;
+ int ret;
+
+ reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
+ SDCA_CTL_GE_SELECTED_MODE, 0);
+
+ ret = regmap_read(interrupt->function_regmap, reg, &val);
+ if (ret) {
+ dev_err(interrupt->dev, "failed to read selected mode: %d\n", ret);
+ return ret;
+ }
+
+ range = sdca_selector_find_range(interrupt->dev, interrupt->entity,
+ SDCA_CTL_GE_SELECTED_MODE,
+ SDCA_SELECTED_MODE_NCOLS, 0);
+ if (!range)
+ return -EINVAL;
+
+ type = sdca_range_search(range, SDCA_SELECTED_MODE_INDEX,
+ val, SDCA_SELECTED_MODE_TERM_TYPE);
+
+ switch (type) {
+ case SDCA_TERM_TYPE_LINEIN_STEREO:
+ case SDCA_TERM_TYPE_LINEIN_FRONT_LR:
+ case SDCA_TERM_TYPE_LINEIN_CENTER_LFE:
+ case SDCA_TERM_TYPE_LINEIN_SURROUND_LR:
+ case SDCA_TERM_TYPE_LINEIN_REAR_LR:
+ report = SND_JACK_LINEIN;
+ break;
+ case SDCA_TERM_TYPE_LINEOUT_STEREO:
+ case SDCA_TERM_TYPE_LINEOUT_FRONT_LR:
+ case SDCA_TERM_TYPE_LINEOUT_CENTER_LFE:
+ case SDCA_TERM_TYPE_LINEOUT_SURROUND_LR:
+ case SDCA_TERM_TYPE_LINEOUT_REAR_LR:
+ report = SND_JACK_LINEOUT;
+ break;
+ case SDCA_TERM_TYPE_MIC_JACK:
+ report = SND_JACK_MICROPHONE;
+ break;
+ case SDCA_TERM_TYPE_HEADPHONE_JACK:
+ report = SND_JACK_HEADPHONE;
+ break;
+ case SDCA_TERM_TYPE_HEADSET_JACK:
+ report = SND_JACK_HEADSET;
+ break;
+ default:
+ break;
+ }
+
+ snd_soc_jack_report(jack_state->jack, report, 0xFFFF);
+
+ return 0;
+}
+EXPORT_SYMBOL_NS_GPL(sdca_jack_report, "SND_SOC_SDCA");
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 3/7] ASoC: SDCA: Add ASoC jack hookup in class driver
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
2025-11-25 15:21 ` [PATCH 1/7] ASoC: SDCA: Factor out jack handling into new c file Charles Keepax
2025-11-25 15:21 ` [PATCH 2/7] ASoC: SDCA: Add ability to connect SDCA jacks to ASoC jacks Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-11-25 15:21 ` [PATCH 4/7] ASoC: SDCA: Add SDCA IRQ enable/disable helpers Charles Keepax
` (3 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
Add the necessary calls to the class driver to connect the ASoC jack
from the machine driver.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
sound/soc/sdca/sdca_class_function.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 0028482a1e752..416948cfb5cb9 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -19,6 +19,7 @@
#include <sound/sdca_fdl.h>
#include <sound/sdca_function.h>
#include <sound/sdca_interrupts.h>
+#include <sound/sdca_jack.h>
#include <sound/sdca_regmap.h>
#include <sound/sdw.h>
#include <sound/soc-component.h>
@@ -195,6 +196,15 @@ static int class_function_component_probe(struct snd_soc_component *component)
return sdca_irq_populate(drv->function, component, core->irq_info);
}
+static int class_function_set_jack(struct snd_soc_component *component,
+ struct snd_soc_jack *jack, void *d)
+{
+ struct class_function_drv *drv = snd_soc_component_get_drvdata(component);
+ struct sdca_class_drv *core = drv->core;
+
+ return sdca_jack_set_jack(core->irq_info, jack);
+}
+
static const struct snd_soc_component_driver class_function_component_drv = {
.probe = class_function_component_probe,
.endianness = 1,
@@ -351,6 +361,9 @@ static int class_function_probe(struct auxiliary_device *auxdev,
return dev_err_probe(dev, PTR_ERR(drv->regmap),
"failed to create regmap");
+ if (desc->type == SDCA_FUNCTION_TYPE_UAJ)
+ cmp_drv->set_jack = class_function_set_jack;
+
ret = sdca_asoc_populate_component(dev, drv->function, cmp_drv,
&dais, &num_dais,
&class_function_sdw_ops);
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 4/7] ASoC: SDCA: Add SDCA IRQ enable/disable helpers
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
` (2 preceding siblings ...)
2025-11-25 15:21 ` [PATCH 3/7] ASoC: SDCA: Add ASoC jack hookup in class driver Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-12-09 12:03 ` Pierre-Louis Bossart
2025-11-25 15:21 ` [PATCH 5/7] ASoC: SDCA: Add basic system suspend support Charles Keepax
` (2 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
Add helpers to enable and disable the SDCA IRQs by Function. These are
useful to sequence the powering down and up around system suspend.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
include/sound/sdca_interrupts.h | 5 +++
sound/soc/sdca/sdca_interrupts.c | 55 ++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
diff --git a/include/sound/sdca_interrupts.h b/include/sound/sdca_interrupts.h
index 8f13417d129ab..abd39e831b63c 100644
--- a/include/sound/sdca_interrupts.h
+++ b/include/sound/sdca_interrupts.h
@@ -84,4 +84,9 @@ int sdca_irq_populate(struct sdca_function_data *function,
struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
struct regmap *regmap, int irq);
+void sdca_irq_enable(struct sdca_function_data *function,
+ struct sdca_interrupt_info *info, bool early);
+void sdca_irq_disable(struct sdca_function_data *function,
+ struct sdca_interrupt_info *info);
+
#endif
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index ff3a7e405fdcb..34bcd18072768 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -541,3 +541,58 @@ struct sdca_interrupt_info *sdca_irq_allocate(struct device *sdev,
return info;
}
EXPORT_SYMBOL_NS_GPL(sdca_irq_allocate, "SND_SOC_SDCA");
+
+/**
+ * sdca_irq_enable - Re-enable SDCA IRQs for a given function
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ * @early: Boolean indicating if early or late IRQs should be enabled.
+ */
+void sdca_irq_enable(struct sdca_function_data *function,
+ struct sdca_interrupt_info *info, bool early)
+{
+ struct sdca_interrupt *interrupt;
+ int i;
+
+ for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
+ interrupt = &info->irqs[i];
+
+ if (!interrupt || interrupt->function != function)
+ continue;
+
+ switch (SDCA_CTL_TYPE(interrupt->entity->type,
+ interrupt->control->sel)) {
+ case SDCA_CTL_TYPE_S(XU, FDL_CURRENTOWNER):
+ if (early)
+ enable_irq(interrupt->irq);
+ break;
+ default:
+ if (!early)
+ enable_irq(interrupt->irq);
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_enable, "SND_SOC_SDCA");
+
+/**
+ * sdca_irq_disable - Disable SDCA IRQs for a given function
+ * @function: Pointer to the SDCA Function.
+ * @info: Pointer to the SDCA interrupt info for this device.
+ */
+void sdca_irq_disable(struct sdca_function_data *function,
+ struct sdca_interrupt_info *info)
+{
+ struct sdca_interrupt *interrupt;
+ int i;
+
+ for (i = 0; i < SDCA_MAX_INTERRUPTS; i++) {
+ interrupt = &info->irqs[i];
+
+ if (!interrupt || interrupt->function != function)
+ continue;
+
+ disable_irq(interrupt->irq);
+ }
+}
+EXPORT_SYMBOL_NS_GPL(sdca_irq_disable, "SND_SOC_SDCA");
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 4/7] ASoC: SDCA: Add SDCA IRQ enable/disable helpers
2025-11-25 15:21 ` [PATCH 4/7] ASoC: SDCA: Add SDCA IRQ enable/disable helpers Charles Keepax
@ 2025-12-09 12:03 ` Pierre-Louis Bossart
0 siblings, 0 replies; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-09 12:03 UTC (permalink / raw)
To: Charles Keepax, broonie
Cc: yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi, shumingf,
linux-sound, patches
On 11/25/25 15:21, Charles Keepax wrote:
> Add helpers to enable and disable the SDCA IRQs by Function. These are
> useful to sequence the powering down and up around system suspend.
Can you elaborate on what the system suspend sequences might be, and how it would differ from the pm_suspend case?
The patch itself looks fine but the design isn't self-explanatory - at least to me. IIRC we already disable interrupts at the host level to prevent interrupts from getting in the way of system suspend.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
` (3 preceding siblings ...)
2025-11-25 15:21 ` [PATCH 4/7] ASoC: SDCA: Add SDCA IRQ enable/disable helpers Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-12-09 12:11 ` Pierre-Louis Bossart
2025-11-25 15:21 ` [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process Charles Keepax
2025-11-25 15:21 ` [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation Charles Keepax
6 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
Add basic system suspend support. Disable the IRQs and force runtime
suspend, during system suspend, because the device will likely fully
power down during suspend.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
sound/soc/sdca/sdca_class.c | 33 ++++++++++++++++++++++
sound/soc/sdca/sdca_class_function.c | 41 ++++++++++++++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 349d32933ba85..438695291257e 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -238,6 +238,38 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
return 0;
}
+static int class_suspend(struct device *dev)
+{
+ struct sdca_class_drv *drv = dev_get_drvdata(dev);
+ int ret;
+
+ disable_irq(drv->sdw->irq);
+
+ ret = pm_runtime_force_suspend(dev);
+ if (ret) {
+ dev_err(dev, "Failed to force suspend: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int class_resume(struct device *dev)
+{
+ struct sdca_class_drv *drv = dev_get_drvdata(dev);
+ int ret;
+
+ ret = pm_runtime_force_resume(dev);
+ if (ret) {
+ dev_err(dev, "Failed to force resume: %d\n", ret);
+ return ret;
+ }
+
+ enable_irq(drv->sdw->irq);
+
+ return 0;
+}
+
static int class_runtime_suspend(struct device *dev)
{
struct sdca_class_drv *drv = dev_get_drvdata(dev);
@@ -278,6 +310,7 @@ static int class_runtime_resume(struct device *dev)
}
static const struct dev_pm_ops class_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(class_suspend, class_resume)
RUNTIME_PM_OPS(class_runtime_suspend, class_runtime_resume, NULL)
};
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index 416948cfb5cb9..dccd0e531cefb 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -33,6 +33,7 @@ struct class_function_drv {
struct sdca_class_drv *core;
struct sdca_function_data *function;
+ bool suspended;
};
static void class_function_regmap_lock(void *data)
@@ -417,6 +418,13 @@ static int class_function_runtime_resume(struct device *dev)
regcache_mark_dirty(drv->regmap);
regcache_cache_only(drv->regmap, false);
+ if (drv->suspended) {
+ sdca_irq_enable(drv->function, drv->core->irq_info, true);
+ sdca_irq_enable(drv->function, drv->core->irq_info, false);
+
+ drv->suspended = false;
+ }
+
ret = regcache_sync(drv->regmap);
if (ret) {
dev_err(drv->dev, "failed to restore register cache: %d\n", ret);
@@ -431,7 +439,40 @@ static int class_function_runtime_resume(struct device *dev)
return ret;
}
+static int class_function_suspend(struct device *dev)
+{
+ struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+ struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
+ int ret;
+
+ drv->suspended = true;
+
+ sdca_irq_disable(drv->function, drv->core->irq_info);
+
+ ret = pm_runtime_force_suspend(dev);
+ if (ret) {
+ dev_err(dev, "Failed to force suspend: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int class_function_resume(struct device *dev)
+{
+ int ret;
+
+ ret = pm_runtime_force_resume(dev);
+ if (ret) {
+ dev_err(dev, "Failed to force resume: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
static const struct dev_pm_ops class_function_pm_ops = {
+ SYSTEM_SLEEP_PM_OPS(class_function_suspend, class_function_resume)
RUNTIME_PM_OPS(class_function_runtime_suspend,
class_function_runtime_resume, NULL)
};
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-11-25 15:21 ` [PATCH 5/7] ASoC: SDCA: Add basic system suspend support Charles Keepax
@ 2025-12-09 12:11 ` Pierre-Louis Bossart
2025-12-10 14:43 ` Charles Keepax
0 siblings, 1 reply; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-09 12:11 UTC (permalink / raw)
To: Charles Keepax, broonie
Cc: yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi, shumingf,
linux-sound, patches
On 11/25/25 15:21, Charles Keepax wrote:
> Add basic system suspend support. Disable the IRQs and force runtime
> suspend, during system suspend, because the device will likely fully
> power down during suspend.
Power-down during system suspend (seems natural) or power-down during pm_runtime suspend (depends on what the host does during clock_stop)?
A quick rewrite would help...
> + if (drv->suspended) {
> + sdca_irq_enable(drv->function, drv->core->irq_info, true);
> + sdca_irq_enable(drv->function, drv->core->irq_info, false);
and a comment here wouldn't hurt, not sure about the side/racy effects of turning the interrupt on before turning it off?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-12-09 12:11 ` Pierre-Louis Bossart
@ 2025-12-10 14:43 ` Charles Keepax
2025-12-10 16:48 ` Charles Keepax
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Charles Keepax @ 2025-12-10 14:43 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On Tue, Dec 09, 2025 at 12:11:27PM +0000, Pierre-Louis Bossart wrote:
> On 11/25/25 15:21, Charles Keepax wrote:
> > Add basic system suspend support. Disable the IRQs and force runtime
> > suspend, during system suspend, because the device will likely fully
> > power down during suspend.
>
> Power-down during system suspend (seems natural) or power-down
> during pm_runtime suspend (depends on what the host does during
> clock_stop)?
At the moment this is written to support FDL after system
suspend only. Which is fairly typical of most devices I have seen
(Cirrus and non-Cirrus). Generally as runtime suspends happen
so often it is preferred not to download firmware there due to
time constraints.
This is definitely up for debate, my primary issue with allowing
firmware download at the runtime level is that SDCA gives you no
way to tell that the device is ready to rock. The only way I have
been able to divine to do this is to wait for an FDL irq and if
one doesn't come within a reasonable time out move on. However,
that waiting would add a considerable delay to runtime resume
even if no firmware was downloaded, which feels problematic.
I guess my two questions would be:
1) Do we really want to support downloading firmware on runtime
suspend? I am doubtful it is really usable due to latency.
2) If we do, do you have any ideas about how to determine if the
device needs firmware?
> > + if (drv->suspended) {
> > + sdca_irq_enable(drv->function, drv->core->irq_info, true);
> > + sdca_irq_enable(drv->function, drv->core->irq_info, false);
>
> and a comment here wouldn't hurt, not sure about the side/racy
> effects of turning the interrupt on before turning it off?
This is perhaps a little misleading the last parameter here is an
early flag not an enable/disable. I could perhaps replace that
with some defines rather than a bool to keep the calls more
self-documenting.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-12-10 14:43 ` Charles Keepax
@ 2025-12-10 16:48 ` Charles Keepax
2025-12-11 10:33 ` Vinod Koul
2025-12-20 11:31 ` Pierre-Louis Bossart
2 siblings, 0 replies; 25+ messages in thread
From: Charles Keepax @ 2025-12-10 16:48 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On Wed, Dec 10, 2025 at 02:43:17PM +0000, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:11:27PM +0000, Pierre-Louis Bossart wrote:
> > On 11/25/25 15:21, Charles Keepax wrote:
> > > + if (drv->suspended) {
> > > + sdca_irq_enable(drv->function, drv->core->irq_info, true);
> > > + sdca_irq_enable(drv->function, drv->core->irq_info, false);
> >
> > and a comment here wouldn't hurt, not sure about the side/racy
> > effects of turning the interrupt on before turning it off?
>
> This is perhaps a little misleading the last parameter here is an
> early flag not an enable/disable. I could perhaps replace that
> with some defines rather than a bool to keep the calls more
> self-documenting.
Look at this again I think best would just be to add an _early
version of the function that is what we did for the populate
functions so it would keep the API more consistent. I will update
for a v2 once the merge window closes.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-12-10 14:43 ` Charles Keepax
2025-12-10 16:48 ` Charles Keepax
@ 2025-12-11 10:33 ` Vinod Koul
2025-12-11 11:28 ` Charles Keepax
2025-12-20 11:31 ` Pierre-Louis Bossart
2 siblings, 1 reply; 25+ messages in thread
From: Vinod Koul @ 2025-12-11 10:33 UTC (permalink / raw)
To: Charles Keepax
Cc: Pierre-Louis Bossart, broonie, yung-chuan.liao, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
On 10-12-25, 14:43, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:11:27PM +0000, Pierre-Louis Bossart wrote:
> > On 11/25/25 15:21, Charles Keepax wrote:
>
> This is definitely up for debate, my primary issue with allowing
> firmware download at the runtime level is that SDCA gives you no
> way to tell that the device is ready to rock. The only way I have
> been able to divine to do this is to wait for an FDL irq and if
> one doesn't come within a reasonable time out move on. However,
> that waiting would add a considerable delay to runtime resume
> even if no firmware was downloaded, which feels problematic.
What is the typical time to have firmware downloaded and get the irq?
> I guess my two questions would be:
> 1) Do we really want to support downloading firmware on runtime
> suspend? I am doubtful it is really usable due to latency.
I feel there is no way around. We cant do this async
Best is to speed up loading by having firmware cached in the kernel so
userspace delays can be avoided (unless ofcourse it is humongous size)
If device needs firmware, we have to download, dont see a choice here
> 2) If we do, do you have any ideas about how to determine if the
> device needs firmware?
Driver should know and first thing in resume, download the firmware,
maybe a flag in device properties?
--
~Vinod
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-12-11 10:33 ` Vinod Koul
@ 2025-12-11 11:28 ` Charles Keepax
0 siblings, 0 replies; 25+ messages in thread
From: Charles Keepax @ 2025-12-11 11:28 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, broonie, yung-chuan.liao, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
On Thu, Dec 11, 2025 at 04:03:02PM +0530, Vinod Koul wrote:
> On 10-12-25, 14:43, Charles Keepax wrote:
> > On Tue, Dec 09, 2025 at 12:11:27PM +0000, Pierre-Louis Bossart wrote:
> > > On 11/25/25 15:21, Charles Keepax wrote:
> > This is definitely up for debate, my primary issue with allowing
> > firmware download at the runtime level is that SDCA gives you no
> > way to tell that the device is ready to rock. The only way I have
> > been able to divine to do this is to wait for an FDL irq and if
> > one doesn't come within a reasonable time out move on. However,
> > that waiting would add a considerable delay to runtime resume
> > even if no firmware was downloaded, which feels problematic.
>
> What is the typical time to have firmware downloaded and get the irq?
There isn't really an answer to that, it will vary alot based on
the host, the firmware size, the device itself. The timeout we
use to wait for the FDL IRQ in the kernel is currently 100mS, but
that isn't determined by the specification so may need extended
when new hardware arrives. For our device on a MTL UPX, with the
current firmware, and that 100mS timeout, it takes just over 300mS
to complete FDL.
> > 1) Do we really want to support downloading firmware on runtime
> > suspend? I am doubtful it is really usable due to latency.
>
> I feel there is no way around. We cant do this async
Obviously agree, if we need to download firmware we have to,
but I think you misunderstand my point. Currently, we don't
have anything that requires this, so why not add support when
we do? I think there is a reasonable chance no one will use the
feature and even if I did add support now, it won't be tested.
Furthermore, there is nothing in the current code that prevents
adding support for this in the future.
> Best is to speed up loading by having firmware cached in the kernel so
> userspace delays can be avoided (unless ofcourse it is humongous size)
> If device needs firmware, we have to download, dont see a choice here
Caching the firmware makes very minimal difference (on my system
pull firmware from user-space takes less than 1% of the time),
the majority of the time is a) spend pumping data through the bus,
b) waiting to ensure are no new FDL IRQs.
> > 2) If we do, do you have any ideas about how to determine if the
> > device needs firmware?
>
> Driver should know and first thing in resume, download the firmware,
> maybe a flag in device properties?
Yeah I think that would be about my best guess at how we would
could reduce the impact of this change. We could add a quirk to
say skip runtime download for this device/system. That would mean
new hardware would work albeit a bit sluggishly and established
systems/devices could quirk for a reasonable user-experience. But
I can't say I love that as a solution.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/7] ASoC: SDCA: Add basic system suspend support
2025-12-10 14:43 ` Charles Keepax
2025-12-10 16:48 ` Charles Keepax
2025-12-11 10:33 ` Vinod Koul
@ 2025-12-20 11:31 ` Pierre-Louis Bossart
2 siblings, 0 replies; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-20 11:31 UTC (permalink / raw)
To: Charles Keepax
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On 12/10/25 15:43, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:11:27PM +0000, Pierre-Louis Bossart wrote:
>> On 11/25/25 15:21, Charles Keepax wrote:
>>> Add basic system suspend support. Disable the IRQs and force runtime
>>> suspend, during system suspend, because the device will likely fully
>>> power down during suspend.
>>
>> Power-down during system suspend (seems natural) or power-down
>> during pm_runtime suspend (depends on what the host does during
>> clock_stop)?
>
> At the moment this is written to support FDL after system
> suspend only. Which is fairly typical of most devices I have seen
> (Cirrus and non-Cirrus). Generally as runtime suspends happen
> so often it is preferred not to download firmware there due to
> time constraints.
>
> This is definitely up for debate, my primary issue with allowing
> firmware download at the runtime level is that SDCA gives you no
> way to tell that the device is ready to rock. The only way I have
> been able to divine to do this is to wait for an FDL irq and if
> one doesn't come within a reasonable time out move on. However,
> that waiting would add a considerable delay to runtime resume
> even if no firmware was downloaded, which feels problematic.
>
> I guess my two questions would be:
> 1) Do we really want to support downloading firmware on runtime
> suspend? I am doubtful it is really usable due to latency.
> 2) If we do, do you have any ideas about how to determine if the
> device needs firmware?
It depends what happens during runtime suspend at the host level.
In the case where the host issues a BUS_RESET even during pm_runtime resume, you will have to re-initialize the device completely, no?
If the runtime suspend only does a clock stop, then presumably there's nothing to do on resume on the device side, i.e. no firmware to reload.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
` (4 preceding siblings ...)
2025-11-25 15:21 ` [PATCH 5/7] ASoC: SDCA: Add basic system suspend support Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-12-09 12:18 ` Pierre-Louis Bossart
2025-11-25 15:21 ` [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation Charles Keepax
6 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
When system suspending the device may be powered off, this means all
state will be lost and the firmware may need to be re-downloaded. Add
the necessary calls to bring the device back up. This also requires that
that the FDL (firmware download) IRQ handler is modified to allow it to
run before runtime PM has been fully restored.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
sound/soc/sdca/sdca_class_function.c | 71 ++++++++++++++++++++--------
sound/soc/sdca/sdca_interrupts.c | 17 +++++--
2 files changed, 62 insertions(+), 26 deletions(-)
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index dccd0e531cefb..aed325087c1dd 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -211,21 +211,12 @@ static const struct snd_soc_component_driver class_function_component_drv = {
.endianness = 1,
};
-static int class_function_boot(struct class_function_drv *drv)
+static int class_function_init_device(struct class_function_drv *drv,
+ unsigned int status)
{
- unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
- SDCA_ENTITY_TYPE_ENTITY_0,
- SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
- unsigned int val;
int ret;
- ret = regmap_read(drv->regmap, reg, &val);
- if (ret < 0) {
- dev_err(drv->dev, "failed to read function status: %d\n", ret);
- return ret;
- }
-
- if (!(val & SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET)) {
+ if (!(status & SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET)) {
dev_dbg(drv->dev, "reset function device\n");
ret = sdca_reset_function(drv->dev, drv->function, drv->regmap);
@@ -233,24 +224,36 @@ static int class_function_boot(struct class_function_drv *drv)
return ret;
}
- if (val & SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION) {
+ if (status & SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION) {
dev_dbg(drv->dev, "write initialisation\n");
ret = sdca_regmap_write_init(drv->dev, drv->core->dev_regmap,
drv->function);
if (ret)
return ret;
+ }
- ret = regmap_write(drv->regmap, reg,
- SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION);
- if (ret < 0) {
- dev_err(drv->dev,
- "failed to clear function init status: %d\n",
- ret);
- return ret;
- }
+ return 0;
+}
+
+static int class_function_boot(struct class_function_drv *drv)
+{
+ unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
+ SDCA_ENTITY_TYPE_ENTITY_0,
+ SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(drv->regmap, reg, &val);
+ if (ret < 0) {
+ dev_err(drv->dev, "failed to read function status: %d\n", ret);
+ return ret;
}
+ ret = class_function_init_device(drv, val);
+ if (ret)
+ return ret;
+
/* Start FDL process */
ret = sdca_irq_populate_early(drv->dev, drv->regmap, drv->function,
drv->core->irq_info);
@@ -419,9 +422,35 @@ static int class_function_runtime_resume(struct device *dev)
regcache_cache_only(drv->regmap, false);
if (drv->suspended) {
+ unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
+ SDCA_ENTITY_TYPE_ENTITY_0,
+ SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
+ unsigned int val;
+
+ ret = regmap_read(drv->regmap, reg, &val);
+ if (ret < 0) {
+ dev_err(drv->dev, "failed to read function status: %d\n", ret);
+ goto err;
+ }
+
+ ret = class_function_init_device(drv, val);
+ if (ret)
+ goto err;
+
sdca_irq_enable(drv->function, drv->core->irq_info, true);
+
+ ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
+ if (ret)
+ goto err;
+
sdca_irq_enable(drv->function, drv->core->irq_info, false);
+ ret = regmap_write(drv->regmap, reg, 0xFF);
+ if (ret < 0) {
+ dev_err(drv->dev, "failed to clear function status: %d\n", ret);
+ goto err;
+ }
+
drv->suspended = false;
}
diff --git a/sound/soc/sdca/sdca_interrupts.c b/sound/soc/sdca/sdca_interrupts.c
index 34bcd18072768..05b4e999c72a4 100644
--- a/sound/soc/sdca/sdca_interrupts.c
+++ b/sound/soc/sdca/sdca_interrupts.c
@@ -205,10 +205,16 @@ static irqreturn_t fdl_owner_handler(int irq, void *data)
irqreturn_t irqret = IRQ_NONE;
int ret;
- ret = pm_runtime_get_sync(dev);
- if (ret < 0) {
- dev_err(dev, "failed to resume for fdl: %d\n", ret);
- goto error;
+ /*
+ * FDL has to run from the system resume handler, at which point
+ * pm_runtime isn't yet active.
+ */
+ if (pm_runtime_active(dev)) {
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to resume for fdl: %d\n", ret);
+ goto error;
+ }
}
ret = sdca_fdl_process(interrupt);
@@ -217,7 +223,8 @@ static irqreturn_t fdl_owner_handler(int irq, void *data)
irqret = IRQ_HANDLED;
error:
- pm_runtime_put(dev);
+ if (pm_runtime_active(dev))
+ pm_runtime_put(dev);
return irqret;
}
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process
2025-11-25 15:21 ` [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process Charles Keepax
@ 2025-12-09 12:18 ` Pierre-Louis Bossart
2025-12-11 11:59 ` Charles Keepax
0 siblings, 1 reply; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-09 12:18 UTC (permalink / raw)
To: Charles Keepax, broonie
Cc: yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi, shumingf,
linux-sound, patches
On 11/25/25 15:21, Charles Keepax wrote:
> When system suspending the device may be powered off, this means all
> state will be lost and the firmware may need to be re-downloaded. Add
> the necessary calls to bring the device back up. This also requires that
> that the FDL (firmware download) IRQ handler is modified to allow it to
> run before runtime PM has been fully restored.
Sounds good indeed.
> sdca_irq_enable(drv->function, drv->core->irq_info, true);
> +
> + ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
> + if (ret)
> + goto err;
> +
> sdca_irq_enable(drv->function, drv->core->irq_info, false);
This answers to my comment on the previous patch but you could make it easier on the reviewer with a better description of what the next patches add.
>
> + ret = regmap_write(drv->regmap, reg, 0xFF);
> + if (ret < 0) {
> + dev_err(drv->dev, "failed to clear function status: %d\n", ret);
> + goto err;
> + }
> +
> + /*
> + * FDL has to run from the system resume handler, at which point
> + * pm_runtime isn't yet active.
> + */
aren't there cases where FDL also needs to run in a pm_runtime resume case?
My understanding of FDL is that the host would *always* check if the device lost context and if firmware is required then the download takes place. I am not sure why we need the assumption that firmware is *only* required for system resume?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process
2025-12-09 12:18 ` Pierre-Louis Bossart
@ 2025-12-11 11:59 ` Charles Keepax
2025-12-20 11:36 ` Pierre-Louis Bossart
0 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-12-11 11:59 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On Tue, Dec 09, 2025 at 12:18:09PM +0000, Pierre-Louis Bossart wrote:
> On 11/25/25 15:21, Charles Keepax wrote:
> > + /*
> > + * FDL has to run from the system resume handler, at which point
> > + * pm_runtime isn't yet active.
> > + */
>
> aren't there cases where FDL also needs to run in a pm_runtime
> resume case?
Potentially, depends if someone builds a system that does that.
> My understanding of FDL is that the host would *always* check
> if the device lost context and if firmware is required then the
> download takes place. I am not sure why we need the assumption
> that firmware is *only* required for system resume?
Mostly because it seems that is pretty standard for PC systems,
to only drop rails on hibernate.
But the primary issue I have is the "check if the device lost
context" part. I don't see a good way to implement that under
SDCA. If we figure out a good way to do that then most of my
objections to allowing the firmware download on runtime resume
go away.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process
2025-12-11 11:59 ` Charles Keepax
@ 2025-12-20 11:36 ` Pierre-Louis Bossart
0 siblings, 0 replies; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-20 11:36 UTC (permalink / raw)
To: Charles Keepax
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On 12/11/25 12:59, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:18:09PM +0000, Pierre-Louis Bossart wrote:
>> On 11/25/25 15:21, Charles Keepax wrote:
>>> + /*
>>> + * FDL has to run from the system resume handler, at which point
>>> + * pm_runtime isn't yet active.
>>> + */
>>
>> aren't there cases where FDL also needs to run in a pm_runtime
>> resume case?
>
> Potentially, depends if someone builds a system that does that.
>
>> My understanding of FDL is that the host would *always* check
>> if the device lost context and if firmware is required then the
>> download takes place. I am not sure why we need the assumption
>> that firmware is *only* required for system resume?
>
> Mostly because it seems that is pretty standard for PC systems,
> to only drop rails on hibernate.
>
> But the primary issue I have is the "check if the device lost
> context" part. I don't see a good way to implement that under
> SDCA. If we figure out a good way to do that then most of my
> objections to allowing the firmware download on runtime resume
> go away.
Wondering if the 'Needs_Initialization' bit could be used but not only initialization writes but firmware as well?
it could be device-specific, but IIRC that was one way to check if context was lost or not.
And there are other bits that tell you if the function was reset, see Function_has_been_reset in the Function_Status control.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation
2025-11-25 15:21 [PATCH 0/7] SDCA jack and system suspend fixups Charles Keepax
` (5 preceding siblings ...)
2025-11-25 15:21 ` [PATCH 6/7] ASoC: SDCA: Device boot into the system suspend process Charles Keepax
@ 2025-11-25 15:21 ` Charles Keepax
2025-12-09 12:20 ` Pierre-Louis Bossart
6 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-11-25 15:21 UTC (permalink / raw)
To: broonie
Cc: yung-chuan.liao, pierre-louis.bossart, vkoul, lgirdwood,
peter.ujfalusi, shumingf, linux-sound, patches
To avoid issues on some devices serialise the boot of each SDCA Function
from the others.
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
sound/soc/sdca/sdca_class.c | 1 +
sound/soc/sdca/sdca_class.h | 2 ++
sound/soc/sdca/sdca_class_function.c | 5 +++++
3 files changed, 8 insertions(+)
diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
index 438695291257e..1b83aad1d9d3b 100644
--- a/sound/soc/sdca/sdca_class.c
+++ b/sound/soc/sdca/sdca_class.c
@@ -205,6 +205,7 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
drv->dev = dev;
drv->sdw = sdw;
mutex_init(&drv->regmap_lock);
+ mutex_init(&drv->init_lock);
dev_set_drvdata(drv->dev, drv);
diff --git a/sound/soc/sdca/sdca_class.h b/sound/soc/sdca/sdca_class.h
index bb4c9dd124296..6f24ea2bbd381 100644
--- a/sound/soc/sdca/sdca_class.h
+++ b/sound/soc/sdca/sdca_class.h
@@ -28,6 +28,8 @@ struct sdca_class_drv {
struct sdca_interrupt_info *irq_info;
struct mutex regmap_lock;
+ /* Serialise function initialisations */
+ struct mutex init_lock;
struct work_struct boot_work;
struct completion device_attach;
diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
index aed325087c1dd..5af13ced9ceff 100644
--- a/sound/soc/sdca/sdca_class_function.c
+++ b/sound/soc/sdca/sdca_class_function.c
@@ -8,6 +8,7 @@
*/
#include <linux/auxiliary_bus.h>
+#include <linux/cleanup.h>
#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/pm.h>
@@ -244,6 +245,8 @@ static int class_function_boot(struct class_function_drv *drv)
unsigned int val;
int ret;
+ guard(mutex)(&drv->core->init_lock);
+
ret = regmap_read(drv->regmap, reg, &val);
if (ret < 0) {
dev_err(drv->dev, "failed to read function status: %d\n", ret);
@@ -418,6 +421,8 @@ static int class_function_runtime_resume(struct device *dev)
struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
int ret;
+ guard(mutex)(&drv->core->init_lock);
+
regcache_mark_dirty(drv->regmap);
regcache_cache_only(drv->regmap, false);
--
2.47.3
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation
2025-11-25 15:21 ` [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation Charles Keepax
@ 2025-12-09 12:20 ` Pierre-Louis Bossart
2025-12-10 15:27 ` Charles Keepax
0 siblings, 1 reply; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-09 12:20 UTC (permalink / raw)
To: Charles Keepax, broonie
Cc: yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi, shumingf,
linux-sound, patches
On 11/25/25 15:21, Charles Keepax wrote:
> To avoid issues on some devices serialise the boot of each SDCA Function
> from the others.
In theory all SDCA functions are independent, can you elaborate on what the problems might be?
I can certainly see that it's not efficient to try and download multiple firmware blobs over the same limited command or BPT/BRA channels, but I am not sure I see the dependencies between functions?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation
2025-12-09 12:20 ` Pierre-Louis Bossart
@ 2025-12-10 15:27 ` Charles Keepax
2025-12-11 10:26 ` Richard Fitzgerald
0 siblings, 1 reply; 25+ messages in thread
From: Charles Keepax @ 2025-12-10 15:27 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On Tue, Dec 09, 2025 at 12:20:41PM +0000, Pierre-Louis Bossart wrote:
> On 11/25/25 15:21, Charles Keepax wrote:
> > To avoid issues on some devices serialise the boot of each SDCA Function
> > from the others.
>
> In theory all SDCA functions are independent, can you elaborate
> on what the problems might be?
How do I put this... hardware and firmware teams are really good
at always considering all the implications of the spec.
> I can certainly see that it's not efficient to try and
> download multiple firmware blobs over the same limited command
> or BPT/BRA channels, but I am not sure I see the dependencies
> between functions?
Generally the dependencies tend to come in from the implementation
not the specification. Whilst logically the functions are all
entirely independent, that is not necessarily how the hardware
will be implemented. I would guess it will be quite common for
the functions to be implemented on a shared back end.
We could implement this as a quirk instead and only apply the
lock for parts that require it, or we could force parts that need
it to use a customer driver. But, it seemed like a) others might
end up with similar requirements, b) the detriment of implementing
it for everyone is quite low, as you observe it's probably better
not to trying to kick off 4 BRA's at one time anyway.
So basically some parts will need it, a fully spec compliant part
would not. But as it has some practical benefits even for spec
compliant parts it seemed better to just add it and see where
that takes us.
Thanks,
Charles
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation
2025-12-10 15:27 ` Charles Keepax
@ 2025-12-11 10:26 ` Richard Fitzgerald
2025-12-20 11:21 ` Pierre-Louis Bossart
0 siblings, 1 reply; 25+ messages in thread
From: Richard Fitzgerald @ 2025-12-11 10:26 UTC (permalink / raw)
To: Charles Keepax, Pierre-Louis Bossart
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On 10/12/2025 3:27 pm, Charles Keepax wrote:
> On Tue, Dec 09, 2025 at 12:20:41PM +0000, Pierre-Louis Bossart wrote:
>> On 11/25/25 15:21, Charles Keepax wrote:
>>> To avoid issues on some devices serialise the boot of each SDCA Function
>>> from the others.
>>
>> In theory all SDCA functions are independent, can you elaborate
>> on what the problems might be?
>
> How do I put this... hardware and firmware teams are really good
> at always considering all the implications of the spec.
>
>> I can certainly see that it's not efficient to try and
>> download multiple firmware blobs over the same limited command
>> or BPT/BRA channels, but I am not sure I see the dependencies
>> between functions?
>
> Generally the dependencies tend to come in from the implementation
> not the specification. Whilst logically the functions are all
> entirely independent, that is not necessarily how the hardware
> will be implemented. I would guess it will be quite common for
> the functions to be implemented on a shared back end.
>
IOW: SDCA assumes all functions are independent, but it is not
always feasible to implement real devices that way. And SDCA does
not tell you whether functions might have dependencies. So the only
safe option is to be pessimistic and assume there might be dependencies.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 7/7] ASoC: SDCA: Add lock to serialise the Function initialisation
2025-12-11 10:26 ` Richard Fitzgerald
@ 2025-12-20 11:21 ` Pierre-Louis Bossart
0 siblings, 0 replies; 25+ messages in thread
From: Pierre-Louis Bossart @ 2025-12-20 11:21 UTC (permalink / raw)
To: Richard Fitzgerald, Charles Keepax
Cc: broonie, yung-chuan.liao, vkoul, lgirdwood, peter.ujfalusi,
shumingf, linux-sound, patches
On 12/11/25 11:26, Richard Fitzgerald wrote:
> On 10/12/2025 3:27 pm, Charles Keepax wrote:
>> On Tue, Dec 09, 2025 at 12:20:41PM +0000, Pierre-Louis Bossart wrote:
>>> On 11/25/25 15:21, Charles Keepax wrote:
>>>> To avoid issues on some devices serialise the boot of each SDCA Function
>>>> from the others.
>>>
>>> In theory all SDCA functions are independent, can you elaborate
>>> on what the problems might be?
>>
>> How do I put this... hardware and firmware teams are really good
>> at always considering all the implications of the spec.
>>
>>> I can certainly see that it's not efficient to try and
>>> download multiple firmware blobs over the same limited command
>>> or BPT/BRA channels, but I am not sure I see the dependencies
>>> between functions?
>>
>> Generally the dependencies tend to come in from the implementation
>> not the specification. Whilst logically the functions are all
>> entirely independent, that is not necessarily how the hardware
>> will be implemented. I would guess it will be quite common for
>> the functions to be implemented on a shared back end.
>>
>
> IOW: SDCA assumes all functions are independent, but it is not
> always feasible to implement real devices that way. And SDCA does
> not tell you whether functions might have dependencies. So the only
> safe option is to be pessimistic and assume there might be dependencies.
Fair enough, but then how would we model these dependencies if there are independent child drivers, one per function? How would we know FunctionA needs to be probed first and do things so that FunctionB becomes operational. Or that FunctionA needs to remain powered so that FunctionB is usable.
I am not pushing back on the existence of such dependencies, just stating the obvious that the current model with child devices/drivers does not provide any hooks to model such real-world dependencies. And it's not even clear if any ACPI information provides such information...
^ permalink raw reply [flat|nested] 25+ messages in thread