* [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface
2026-05-15 15:10 [PATCH 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
@ 2026-05-15 15:10 ` Valerio Setti
2026-05-15 15:36 ` sashiko-bot
2026-05-15 15:10 ` [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures Valerio Setti
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Valerio Setti @ 2026-05-15 15:10 UTC (permalink / raw)
To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
Cc: linux-kernel, linux-sound, linux-arm-kernel, linux-amlogic,
Valerio Setti
These files are the basic block which allow to shape I2S in GX devices
the same as the AXG ones: the DAI backend only controls the interface
(i.e. clocks and pins) whereas a formatter takes care of properly
formatting the data.
gx-formatter and gx-interface are strongly inspired to axg-tdm-formatter
and axg-tdm, respectively. The long term plan is to join the two platforms
to use the same formatter solution.
There is only a minor addition here compared to what has been done for
AXG and it's "gx_formatter_create()" which is required in order to let
already existing AIU code to make use of this formatter without making any
devicetree change.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
sound/soc/meson/Makefile | 1 +
sound/soc/meson/gx-formatter.c | 277 +++++++++++++++++++++++++++++++++++++++++
sound/soc/meson/gx-formatter.h | 47 +++++++
sound/soc/meson/gx-interface.h | 45 +++++++
4 files changed, 370 insertions(+)
diff --git a/sound/soc/meson/Makefile b/sound/soc/meson/Makefile
index 24078e4396b02d545d8ba4bcb1632979001354e3..146ec81526ba091a174a113ce3d8412ddbbfd9dd 100644
--- a/sound/soc/meson/Makefile
+++ b/sound/soc/meson/Makefile
@@ -4,6 +4,7 @@ snd-soc-meson-aiu-y := aiu.o
snd-soc-meson-aiu-y += aiu-acodec-ctrl.o
snd-soc-meson-aiu-y += aiu-codec-ctrl.o
snd-soc-meson-aiu-y += aiu-encoder-i2s.o
+snd-soc-meson-aiu-y += gx-formatter.o
snd-soc-meson-aiu-y += aiu-encoder-spdif.o
snd-soc-meson-aiu-y += aiu-fifo.o
snd-soc-meson-aiu-y += aiu-fifo-i2s.o
diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
new file mode 100644
index 0000000000000000000000000000000000000000..db93546ed9217f711fcdbeddbd815ce21f27bab5
--- /dev/null
+++ b/sound/soc/meson/gx-formatter.c
@@ -0,0 +1,277 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+//
+// Copyright (c) 2026 BayLibre, SAS.
+// Author: Valerio Setti <vsetti@baylibre.com>
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <sound/soc.h>
+
+#include "gx-formatter.h"
+
+struct gx_formatter {
+ struct list_head list;
+ struct gx_stream *stream;
+ const struct gx_formatter_driver *drv;
+ bool enabled;
+ struct regmap *map;
+};
+
+static int gx_formatter_enable(struct gx_formatter *formatter)
+{
+ int ret;
+
+ /* Do nothing if the formatter is already enabled */
+ if (formatter->enabled)
+ return 0;
+
+ /* Setup the stream parameter in the formatter */
+ if (formatter->drv->ops->prepare) {
+ ret = formatter->drv->ops->prepare(formatter->map,
+ formatter->drv->quirks,
+ formatter->stream);
+ if (ret)
+ return ret;
+ }
+
+ /* Finally, actually enable the formatter */
+ if (formatter->drv->ops->enable)
+ formatter->drv->ops->enable(formatter->map);
+
+ formatter->enabled = true;
+
+ return 0;
+}
+
+static void gx_formatter_disable(struct gx_formatter *formatter)
+{
+ /* Do nothing if the formatter is already disabled */
+ if (!formatter->enabled)
+ return;
+
+ if (formatter->drv->ops->disable)
+ formatter->drv->ops->disable(formatter->map);
+
+ formatter->enabled = false;
+}
+
+static int gx_formatter_attach(struct gx_formatter *formatter)
+{
+ struct gx_stream *ts = formatter->stream;
+ int ret = 0;
+
+ mutex_lock(&ts->lock);
+
+ /* Catch up if the stream is already running when we attach */
+ if (ts->ready) {
+ ret = gx_formatter_enable(formatter);
+ if (ret) {
+ pr_err("failed to enable formatter\n");
+ goto out;
+ }
+ }
+
+ list_add_tail(&formatter->list, &ts->formatter_list);
+out:
+ mutex_unlock(&ts->lock);
+ return ret;
+}
+
+static void gx_formatter_detach(struct gx_formatter *formatter)
+{
+ struct gx_stream *ts = formatter->stream;
+
+ mutex_lock(&ts->lock);
+ list_del(&formatter->list);
+ mutex_unlock(&ts->lock);
+
+ gx_formatter_disable(formatter);
+}
+
+static int gx_formatter_power_up(struct gx_formatter *formatter,
+ struct snd_soc_dapm_widget *w)
+{
+ struct gx_stream *ts = formatter->drv->ops->get_stream(w);
+ int ret;
+
+ /*
+ * If we don't get a stream at this stage, it would mean that the
+ * widget is powering up but is not attached to any backend DAI.
+ * It should not happen, ever !
+ */
+ if (WARN_ON(!ts))
+ return -ENODEV;
+
+ formatter->stream = ts;
+ ret = gx_formatter_attach(formatter);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void gx_formatter_power_down(struct gx_formatter *formatter)
+{
+ gx_formatter_detach(formatter);
+ formatter->stream = NULL;
+}
+
+int gx_formatter_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *control,
+ int event)
+{
+ struct snd_soc_component *c;
+ struct gx_formatter *formatter;
+ int ret = 0;
+
+ c = snd_soc_dapm_to_component(w->dapm);
+
+ if (w->priv)
+ formatter = w->priv;
+ else
+ formatter = snd_soc_component_get_drvdata(c);
+
+ switch (event) {
+ case SND_SOC_DAPM_PRE_PMU:
+ ret = gx_formatter_power_up(formatter, w);
+ break;
+
+ case SND_SOC_DAPM_PRE_PMD:
+ gx_formatter_power_down(formatter);
+ break;
+
+ default:
+ dev_err(c->dev, "Unexpected event %d\n", event);
+ return -EINVAL;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gx_formatter_event);
+
+int gx_formatter_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ const struct gx_formatter_driver *drv;
+ struct gx_formatter *formatter;
+ void __iomem *regs;
+
+ drv = of_device_get_match_data(dev);
+ if (!drv) {
+ dev_err(dev, "failed to match device\n");
+ return -ENODEV;
+ }
+
+ formatter = devm_kzalloc(dev, sizeof(*formatter), GFP_KERNEL);
+ if (!formatter)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, formatter);
+ formatter->drv = drv;
+
+ regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+
+ formatter->map = devm_regmap_init_mmio(dev, regs, drv->regmap_cfg);
+ if (IS_ERR(formatter->map)) {
+ dev_err(dev, "failed to init regmap: %ld\n",
+ PTR_ERR(formatter->map));
+ return PTR_ERR(formatter->map);
+ }
+
+ return devm_snd_soc_register_component(dev, drv->component_drv,
+ NULL, 0);
+}
+EXPORT_SYMBOL_GPL(gx_formatter_probe);
+
+int gx_formatter_create(struct device *dev,
+ struct snd_soc_dapm_widget *w,
+ const struct gx_formatter_driver *drv,
+ struct regmap *regmap)
+{
+ struct gx_formatter *formatter;
+
+ formatter = devm_kzalloc(dev, sizeof(*formatter), GFP_KERNEL);
+ if (!formatter)
+ return -ENOMEM;
+
+ formatter->drv = drv;
+ formatter->map = regmap;
+
+ w->priv = formatter;
+
+ return 0;
+}
+
+int gx_stream_start(struct gx_stream *ts)
+{
+ struct gx_formatter *formatter;
+ int ret = 0;
+
+ mutex_lock(&ts->lock);
+
+ /* Start all the formatters attached to the stream */
+ list_for_each_entry(formatter, &ts->formatter_list, list) {
+ ret = gx_formatter_enable(formatter);
+ if (ret) {
+ pr_err("failed to start tdm stream\n");
+ goto out;
+ }
+ }
+
+ ts->ready = true;
+
+out:
+ mutex_unlock(&ts->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(gx_stream_start);
+
+void gx_stream_stop(struct gx_stream *ts)
+{
+ struct gx_formatter *formatter;
+
+ mutex_lock(&ts->lock);
+ ts->ready = false;
+
+ /* Stop all the formatters attached to the stream */
+ list_for_each_entry(formatter, &ts->formatter_list, list) {
+ gx_formatter_disable(formatter);
+ }
+
+ mutex_unlock(&ts->lock);
+}
+EXPORT_SYMBOL_GPL(gx_stream_stop);
+
+struct gx_stream *gx_stream_alloc(struct gx_iface *iface)
+{
+ struct gx_stream *ts;
+
+ ts = kzalloc(sizeof(*ts), GFP_KERNEL);
+ if (ts) {
+ INIT_LIST_HEAD(&ts->formatter_list);
+ mutex_init(&ts->lock);
+ ts->iface = iface;
+ }
+
+ return ts;
+}
+EXPORT_SYMBOL_GPL(gx_stream_alloc);
+
+void gx_stream_free(struct gx_stream *ts)
+{
+ /*
+ * If the list is not empty, it would mean that one of the formatter
+ * widget is still powered and attached to the interface while we
+ * are removing the TDM DAI. It should not be possible
+ */
+ WARN_ON(!list_empty(&ts->formatter_list));
+ mutex_destroy(&ts->lock);
+ kfree(ts);
+}
+EXPORT_SYMBOL_GPL(gx_stream_free);
+
+MODULE_DESCRIPTION("Amlogic GX formatter driver");
+MODULE_AUTHOR("Valerio Setti <vsetti@baylibre.com>");
+MODULE_LICENSE("GPL");
diff --git a/sound/soc/meson/gx-formatter.h b/sound/soc/meson/gx-formatter.h
new file mode 100644
index 0000000000000000000000000000000000000000..05670c3dfb9f43ac3ee959f1d3d11bacee020c43
--- /dev/null
+++ b/sound/soc/meson/gx-formatter.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2026 Baylibre SAS.
+ * Author: Valerio Setti <vsetti@baylibre.com>
+ */
+
+#ifndef _MESON_GX_FORMATTER_H
+#define _MESON_GX_FORMATTER_H
+
+#include "gx-interface.h"
+
+struct platform_device;
+struct regmap;
+struct snd_soc_dapm_widget;
+struct snd_kcontrol;
+
+struct gx_formatter_hw {
+ unsigned int skew_offset;
+};
+
+struct gx_formatter_ops {
+ struct gx_stream *(*get_stream)(struct snd_soc_dapm_widget *w);
+ void (*enable)(struct regmap *map);
+ void (*disable)(struct regmap *map);
+ int (*prepare)(struct regmap *map,
+ const struct gx_formatter_hw *quirks,
+ struct gx_stream *ts);
+};
+
+struct gx_formatter_driver {
+ const struct snd_soc_component_driver *component_drv;
+ const struct regmap_config *regmap_cfg;
+ const struct gx_formatter_ops *ops;
+ const struct gx_formatter_hw *quirks;
+};
+
+int gx_formatter_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *control,
+ int event);
+int gx_formatter_probe(struct platform_device *pdev);
+
+int gx_formatter_create(struct device *dev,
+ struct snd_soc_dapm_widget *w,
+ const struct gx_formatter_driver *drv,
+ struct regmap *regmap);
+
+#endif /* _MESON_GX_FORMATTER_H */
diff --git a/sound/soc/meson/gx-interface.h b/sound/soc/meson/gx-interface.h
new file mode 100644
index 0000000000000000000000000000000000000000..d9ab894589fa039f7fdd76765390630a2c6d8fbf
--- /dev/null
+++ b/sound/soc/meson/gx-interface.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+/*
+ * Copyright (c) 2026 Baylibre SAS.
+ * Author: Valerio Setti <vsetti@baylibre.com>
+ */
+
+#ifndef _MESON_GX_INTERFACE_H
+#define _MESON_GX_INTERFACE_H
+
+#include <linux/clk.h>
+#include <linux/regmap.h>
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+
+struct gx_iface {
+ struct clk *mclk;
+ unsigned long mclk_rate;
+
+ /* format is common to all the DAIs of the iface */
+ unsigned int fmt;
+
+ /* For component wide symmetry */
+ int rate;
+};
+
+struct gx_stream {
+ struct gx_iface *iface;
+ struct list_head formatter_list;
+ struct mutex lock;
+ unsigned int channels;
+ unsigned int width;
+ unsigned int physical_width;
+ bool ready;
+
+ /* For continuous clock tracking */
+ bool clk_enabled;
+};
+
+struct gx_stream *gx_stream_alloc(struct gx_iface *iface);
+void gx_stream_free(struct gx_stream *ts);
+int gx_stream_start(struct gx_stream *ts);
+void gx_stream_stop(struct gx_stream *ts);
+
+#endif /* _MESON_GX_INTERFACE_H */
--
2.39.5
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface
2026-05-15 15:10 ` [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface Valerio Setti
@ 2026-05-15 15:36 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-05-15 15:36 UTC (permalink / raw)
To: Valerio Setti; +Cc: linux-amlogic, neil.armstrong
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unhandled error paths during widget power-up leave the formatter in an incomplete state, causing deterministic kernel panics during power-down.
- [High] Missing rollback of enabled formatters in `gx_stream_start` failure path leads to hardware state leaks.
--
commit 37f14cf52933f802dadd48fb50c3876ad1e0e2c5
Author: Valerio Setti <vsetti@baylibre.com>
Subject: ASoC: meson: gx: add gx-formatter and gx-interface
This commit adds basic formatter and interface blocks to shape I2S in GX
devices. It introduces gx_formatter_create() to allow existing AIU code to
utilize the formatter without devicetree modifications.
> diff --git a/sound/soc/meson/gx-formatter.c b/sound/soc/meson/gx-formatter.c
> new file mode 100644
> index 0000000000000..db93546ed9217
> --- /dev/null
> +++ b/sound/soc/meson/gx-formatter.c
[ ... ]
> +static int gx_formatter_power_up(struct gx_formatter *formatter,
> + struct snd_soc_dapm_widget *w)
> +{
> + struct gx_stream *ts = formatter->drv->ops->get_stream(w);
> + int ret;
> +
> + /*
> + * If we don't get a stream at this stage, it would mean that the
> + * widget is powering up but is not attached to any backend DAI.
> + * It should not happen, ever !
> + */
> + if (WARN_ON(!ts))
> + return -ENODEV;
Will this cause a NULL pointer dereference later if the warning triggers?
Since ASoC DAPM core ignores error codes returned by widget PRE_PMU events,
the widget will still be considered powered up and the PRE_PMD event will
eventually execute when the stream stops.
If this returns -ENODEV here, formatter->stream remains NULL. When PRE_PMD
triggers gx_formatter_power_down() and then gx_formatter_detach(), the
detach function will attempt to lock ts->lock where ts is NULL.
> +
> + formatter->stream = ts;
> + ret = gx_formatter_attach(formatter);
> + if (ret)
> + return ret;
Could this error path also lead to a crash during the PRE_PMD teardown?
If gx_formatter_attach() fails, it skips adding formatter->list to the
stream's formatter_list. Later, when the stream stops and PRE_PMD is invoked,
gx_formatter_detach() unconditionally calls list_del(&formatter->list).
Since the list was never initialized with INIT_LIST_HEAD() or added to the
list, calling list_del() on it will dereference uninitialized pointers.
> +
> + return 0;
> +}
[ ... ]
> +int gx_stream_start(struct gx_stream *ts)
> +{
> + struct gx_formatter *formatter;
> + int ret = 0;
> +
> + mutex_lock(&ts->lock);
> +
> + /* Start all the formatters attached to the stream */
> + list_for_each_entry(formatter, &ts->formatter_list, list) {
> + ret = gx_formatter_enable(formatter);
> + if (ret) {
> + pr_err("failed to start tdm stream\n");
> + goto out;
Do we need to disable the formatters that were already successfully enabled
in previous iterations of this loop before returning?
The ALSA core does not automatically invoke the STOP trigger when a START
trigger fails. Returning an error here without rolling back the earlier
formatters could leave the hardware clocks and registers permanently enabled,
resulting in leaked resources.
> + }
> + }
> +
> + ts->ready = true;
> +
> +out:
> + mutex_unlock(&ts->lock);
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com?part=1
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures
2026-05-15 15:10 [PATCH 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
2026-05-15 15:10 ` [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface Valerio Setti
@ 2026-05-15 15:10 ` Valerio Setti
2026-05-15 16:10 ` sashiko-bot
2026-05-15 15:10 ` [PATCH 3/4] ASoC: meson: aiu: introduce I2S output formatter Valerio Setti
2026-05-15 15:10 ` [PATCH 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
3 siblings, 1 reply; 8+ messages in thread
From: Valerio Setti @ 2026-05-15 15:10 UTC (permalink / raw)
To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
Cc: linux-kernel, linux-sound, linux-arm-kernel, linux-amlogic,
Valerio Setti
Start using gx_iface and gx_stream to store interface and stream info,
respectively. probe()/remove() functions are added to allocate/free the
gx_stream structures for each PCM stream.
Clock-wise instead of bulk enabling all the clocks on startup and disabling
them on shutdown, only the peripheral's internal ones are enabled/disabled
in those functions, whereas MCLK and I2S clock divider are handled in
hw_params/hw_free.
Interface wide rate symmetry is also enforced here. This is useful when the
interface is used for playback and capture at the same time.
Finally a trigger() callback is also added to start/stop the associated
I2S data formatter.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
sound/soc/meson/aiu-encoder-i2s.c | 200 ++++++++++++++++++++++++++++++++------
sound/soc/meson/aiu.h | 3 +
2 files changed, 174 insertions(+), 29 deletions(-)
diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index 3b4061508c18047fe8d6f3f98061720f8ce238f2..39accd396affb8beb49fa7cca394244730b24574 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -10,6 +10,8 @@
#include <sound/soc-dai.h>
#include "aiu.h"
+#include "gx-formatter.h"
+#include "gx-interface.h"
#define AIU_I2S_SOURCE_DESC_MODE_8CH BIT(0)
#define AIU_I2S_SOURCE_DESC_MODE_24BIT BIT(5)
@@ -79,7 +81,7 @@ static int aiu_encoder_i2s_setup_desc(struct snd_soc_component *component,
}
static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
- struct snd_pcm_hw_params *params,
+ struct gx_stream *ts,
unsigned int bs)
{
switch (bs) {
@@ -109,7 +111,7 @@ static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
}
static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
- struct snd_pcm_hw_params *params,
+ struct gx_stream *ts,
unsigned int bs)
{
/*
@@ -119,7 +121,7 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
* increased by 50% to get the correct output rate.
* No idea why !
*/
- if (params_width(params) == 16 && params_channels(params) == 8) {
+ if (ts->width == 16 && ts->channels == 8) {
if (bs % 2) {
dev_err(component->dev,
"Cannot increase i2s divider by 50%%\n");
@@ -142,24 +144,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
}
static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
- struct snd_pcm_hw_params *params)
+ struct gx_stream *ts)
{
struct aiu *aiu = snd_soc_component_get_drvdata(component);
- unsigned int srate = params_rate(params);
unsigned int fs, bs;
int ret;
/* Get the oversampling factor */
- fs = DIV_ROUND_CLOSEST(clk_get_rate(aiu->i2s.clks[MCLK].clk), srate);
+ fs = DIV_ROUND_CLOSEST(ts->iface->mclk_rate, ts->iface->rate);
if (fs % 64)
return -EINVAL;
- /* Send data MSB first */
- snd_soc_component_update_bits(component, AIU_I2S_DAC_CFG,
- AIU_I2S_DAC_CFG_MSB_FIRST,
- AIU_I2S_DAC_CFG_MSB_FIRST);
-
/* Set bclk to lrlck ratio */
snd_soc_component_update_bits(component, AIU_CODEC_DAC_LRCLK_CTRL,
AIU_CODEC_DAC_LRCLK_CTRL_DIV,
@@ -169,9 +165,9 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
bs = fs / 64;
if (aiu->platform->has_clk_ctrl_more_i2s_div)
- ret = aiu_encoder_i2s_set_more_div(component, params, bs);
+ ret = aiu_encoder_i2s_set_more_div(component, ts, bs);
else
- ret = aiu_encoder_i2s_set_legacy_div(component, params, bs);
+ ret = aiu_encoder_i2s_set_legacy_div(component, ts, bs);
if (ret)
return ret;
@@ -188,25 +184,55 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
{
+ struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
+ struct gx_iface *iface = ts->iface;
struct snd_soc_component *component = dai->component;
int ret;
- /* Disable the clock while changing the settings */
- aiu_encoder_i2s_divider_enable(component, false);
+ /* Enforce interface wide rate symmetry. */
+ if (iface->rate && (iface->rate != params_rate(params))) {
+ dev_err(dai->dev, "can't set iface rate (%d != %d)\n",
+ iface->rate, params_rate(params));
+ return -EINVAL;
+ }
+
+ iface->rate = params_rate(params);
+ ts->physical_width = params_physical_width(params);
+ ts->width = params_width(params);
+ ts->channels = params_channels(params);
ret = aiu_encoder_i2s_setup_desc(component, params);
if (ret) {
- dev_err(dai->dev, "setting i2s desc failed\n");
+ dev_err(dai->dev, "setting i2s desc failed: %d\n", ret);
return ret;
}
- ret = aiu_encoder_i2s_set_clocks(component, params);
+ ret = aiu_encoder_i2s_set_clocks(component, ts);
if (ret) {
- dev_err(dai->dev, "setting i2s clocks failed\n");
+ dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
return ret;
}
- aiu_encoder_i2s_divider_enable(component, true);
+ return 0;
+}
+
+static int aiu_encoder_i2s_prepare(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
+ struct snd_soc_component *component = dai->component;
+ int ret;
+
+ if (ts->clk_enabled)
+ return 0;
+
+ ret = clk_prepare_enable(ts->iface->mclk);
+ if (ret)
+ return ret;
+
+ ts->clk_enabled = true;
+
+ aiu_encoder_i2s_divider_enable(component, 1);
return 0;
}
@@ -214,9 +240,20 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
+ struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
struct snd_soc_component *component = dai->component;
- aiu_encoder_i2s_divider_enable(component, false);
+ /*
+ * Disable the i2s clock divider only if this is the last substream
+ * being closed.
+ */
+ if (snd_soc_dai_active(dai) <= 1)
+ aiu_encoder_i2s_divider_enable(component, 0);
+
+ if (ts->clk_enabled) {
+ clk_disable_unprepare(ts->iface->mclk);
+ ts->clk_enabled = false;
+ }
return 0;
}
@@ -224,6 +261,8 @@ static int aiu_encoder_i2s_hw_free(struct snd_pcm_substream *substream,
static int aiu_encoder_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
{
struct snd_soc_component *component = dai->component;
+ struct aiu *aiu = snd_soc_component_get_drvdata(component);
+ struct gx_iface *iface = &aiu->i2s.iface;
unsigned int inv = fmt & SND_SOC_DAIFMT_INV_MASK;
unsigned int val = 0;
unsigned int skew;
@@ -255,9 +294,12 @@ static int aiu_encoder_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
skew = 0;
break;
default:
+ dev_err(dai->dev, "unsupported dai format\n");
return -EINVAL;
}
+ iface->fmt = fmt;
+
val |= FIELD_PREP(AIU_CLK_CTRL_LRCLK_SKEW, skew);
snd_soc_component_update_bits(component, AIU_CLK_CTRL,
AIU_CLK_CTRL_LRCLK_INVERT |
@@ -281,10 +323,14 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
return 0;
ret = clk_set_rate(aiu->i2s.clks[MCLK].clk, freq);
- if (ret)
- dev_err(dai->dev, "Failed to set sysclk to %uHz", freq);
+ if (ret) {
+ dev_err(dai->dev, "Failed to set sysclk to %uHz: %d", freq, ret);
+ return ret;
+ }
- return ret;
+ aiu->i2s.iface.mclk_rate = freq;
+
+ return 0;
}
static const unsigned int hw_channels[] = {2, 8};
@@ -305,30 +351,126 @@ static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream,
SNDRV_PCM_HW_PARAM_CHANNELS,
&hw_channel_constraints);
if (ret) {
- dev_err(dai->dev, "adding channels constraints failed\n");
+ dev_err(dai->dev, "adding channels constraints failed: %d\n", ret);
return ret;
}
- ret = clk_bulk_prepare_enable(aiu->i2s.clk_num, aiu->i2s.clks);
- if (ret)
- dev_err(dai->dev, "failed to enable i2s clocks\n");
+ /*
+ * Enable only clocks which are required for the interface internal
+ * logic. MCLK is enabled/disabled from the formatter and the I2S
+ * divider is enabled/disabled in "hw_params"/"hw_free", respectively.
+ */
+ ret = clk_prepare_enable(aiu->i2s.clks[PCLK].clk);
+ if (ret) {
+ dev_err(dai->dev, "failed to enable PCLK: %d\n", ret);
+ return ret;
+ }
+ ret = clk_prepare_enable(aiu->i2s.clks[MIXER].clk);
+ if (ret) {
+ dev_err(dai->dev, "failed to enable MIXER: %d\n", ret);
+ clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
+ return ret;
+ }
+ ret = clk_prepare_enable(aiu->i2s.clks[AOCLK].clk);
+ if (ret) {
+ dev_err(dai->dev, "failed to enable AOCLK: %d\n", ret);
+ clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
+ clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
+ return ret;
+ }
- return ret;
+ return 0;
}
static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
+ struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
+ struct gx_iface *iface = ts->iface;
+
+ if (!snd_soc_dai_active(dai))
+ iface->rate = 0;
+
+ clk_disable_unprepare(aiu->i2s.clks[AOCLK].clk);
+ clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
+ clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
+}
- clk_bulk_disable_unprepare(aiu->i2s.clk_num, aiu->i2s.clks);
+static int aiu_encoder_i2s_trigger(struct snd_pcm_substream *substream,
+ int cmd,
+ struct snd_soc_dai *dai)
+{
+ struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
+ int ret;
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = gx_stream_start(ts);
+ break;
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ case SNDRV_PCM_TRIGGER_STOP:
+ gx_stream_stop(ts);
+ ret = 0;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int aiu_encoder_i2s_remove_dai(struct snd_soc_dai *dai)
+{
+ int stream;
+
+ for_each_pcm_streams(stream) {
+ struct gx_stream *ts = snd_soc_dai_dma_data_get(dai, stream);
+
+ if (ts)
+ gx_stream_free(ts);
+ }
+
+ return 0;
+}
+
+static int aiu_encoder_i2s_probe_dai(struct snd_soc_dai *dai)
+{
+ struct aiu *aiu = snd_soc_dai_get_drvdata(dai);
+ struct gx_iface *iface = &aiu->i2s.iface;
+ int stream;
+
+ for_each_pcm_streams(stream) {
+ struct gx_stream *ts;
+
+ if (!snd_soc_dai_get_widget(dai, stream))
+ continue;
+
+ ts = gx_stream_alloc(iface);
+ if (!ts) {
+ aiu_encoder_i2s_remove_dai(dai);
+ return -ENOMEM;
+ }
+ snd_soc_dai_dma_data_set(dai, stream, ts);
+ }
+
+ iface->mclk = aiu->i2s.clks[MCLK].clk;
+
+ return 0;
}
const struct snd_soc_dai_ops aiu_encoder_i2s_dai_ops = {
+ .probe = aiu_encoder_i2s_probe_dai,
+ .remove = aiu_encoder_i2s_remove_dai,
.hw_params = aiu_encoder_i2s_hw_params,
+ .prepare = aiu_encoder_i2s_prepare,
.hw_free = aiu_encoder_i2s_hw_free,
.set_fmt = aiu_encoder_i2s_set_fmt,
.set_sysclk = aiu_encoder_i2s_set_sysclk,
.startup = aiu_encoder_i2s_startup,
.shutdown = aiu_encoder_i2s_shutdown,
+ .trigger = aiu_encoder_i2s_trigger,
};
diff --git a/sound/soc/meson/aiu.h b/sound/soc/meson/aiu.h
index 0f94c8bf608181112d78402532b832eb50c2d409..68310de0bdf7a97d8de2ff306c159248ee9b0ede 100644
--- a/sound/soc/meson/aiu.h
+++ b/sound/soc/meson/aiu.h
@@ -7,6 +7,8 @@
#ifndef _MESON_AIU_H
#define _MESON_AIU_H
+#include "gx-formatter.h"
+
struct clk;
struct clk_bulk_data;
struct device;
@@ -25,6 +27,7 @@ struct aiu_interface {
struct clk_bulk_data *clks;
unsigned int clk_num;
int irq;
+ struct gx_iface iface;
};
struct aiu_platform_data {
--
2.39.5
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures
2026-05-15 15:10 ` [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures Valerio Setti
@ 2026-05-15 16:10 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-05-15 16:10 UTC (permalink / raw)
To: Valerio Setti; +Cc: neil.armstrong, linux-amlogic
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Double free / Use-After-Free memory corruption on DAI probe failure path.
- [High] Sleeping in atomic context within the PCM trigger callback.
- [High] Custom interface rate symmetry logic permanently locks the sample rate.
- [High] Incomplete parameter symmetry allows concurrent streams to corrupt the shared hardware clock divider.
- [High] Unsafe hardware soft-resets in `hw_params` corrupt active streams.
- [Medium] Data race in manual rate symmetry enforcement.
- [Medium] Integer underflow when programming the clock divider due to uninitialized sysclk.
--
commit ed5bb252934aea5e2205d3b589d2556dcdbed81f
Author: Valerio Setti <vsetti@baylibre.com>
Subject: ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures
This commit transitions the AIU I2S encoder to use gx_iface and gx_stream
structures, moving clock handling and rate symmetry into the hw_params
and trigger callbacks.
> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c180..39accd396affb 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
[ ... ]
> @@ -142,24 +144,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> }
>
> static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
> - struct snd_pcm_hw_params *params)
> + struct gx_stream *ts)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(component);
> - unsigned int srate = params_rate(params);
> unsigned int fs, bs;
> int ret;
>
> /* Get the oversampling factor */
> - fs = DIV_ROUND_CLOSEST(clk_get_rate(aiu->i2s.clks[MCLK].clk), srate);
> + fs = DIV_ROUND_CLOSEST(ts->iface->mclk_rate, ts->iface->rate);
>
> if (fs % 64)
> return -EINVAL;
If the machine driver does not call aiu_encoder_i2s_set_sysclk(),
iface->mclk_rate remains 0. This causes fs to evaluate to 0, which passes
the modulo check since 0 % 64 is 0.
When bs = fs / 64 (which is 0) is passed into aiu_encoder_i2s_set_more_div(),
does bs - 1 underflow to 0xffffffff?
If so, FIELD_PREP(AIU_CLK_CTRL_MORE_I2S_DIV, bs - 1) might mask the
underflowed value and write a corrupted state to the clock control register.
[ ... ]
> @@ -188,25 +184,55 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params,
> struct snd_soc_dai *dai)
> {
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + struct gx_iface *iface = ts->iface;
> struct snd_soc_component *component = dai->component;
> int ret;
>
> - /* Disable the clock while changing the settings */
> - aiu_encoder_i2s_divider_enable(component, false);
The removal of aiu_encoder_i2s_divider_enable(component, false) means
this function now performs pipeline soft-resets (AIU_RST_SOFT_I2S_FAST)
and modifies clock dividers while the hardware might be actively running a
concurrent stream.
Can this corrupt the bit-clock or cause pipeline lockups on the active
stream?
> + /* Enforce interface wide rate symmetry. */
> + if (iface->rate && (iface->rate != params_rate(params))) {
> + dev_err(dai->dev, "can't set iface rate (%d != %d)\n",
> + iface->rate, params_rate(params));
> + return -EINVAL;
> + }
> +
> + iface->rate = params_rate(params);
> + ts->physical_width = params_physical_width(params);
> + ts->width = params_width(params);
> + ts->channels = params_channels(params);
ALSA permits concurrent hw_params ioctls on independent substreams, such
as playback and capture. Since the rate check and assignment are performed
without mutual exclusion, could two threads simultaneously see iface->rate
as 0 and overwrite each other's configuration?
Additionally, this logic enforces symmetry for the sample rate, but ignores
channel count and physical width.
If a playback stream is actively running at 16-bit 2-channel, and a capture
stream is concurrently configured to 16-bit 8-channel, it appears
aiu_encoder_i2s_set_more_div() will recalculate and overwrite the shared
hardware clock divider AIU_CLK_CTRL_MORE_I2S_DIV.
Could this silently corrupt the audio data of the actively playing stream?
[ ... ]
> static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + struct gx_iface *iface = ts->iface;
> +
> + if (!snd_soc_dai_active(dai))
> + iface->rate = 0;
The ASoC core invokes the shutdown callback before decrementing the stream's
active count.
Because of this, does snd_soc_dai_active(dai) always evaluate to at least 1
during the shutdown of the final stream?
If so, iface->rate will never be reset to 0, which would cause subsequent
streams with different sample rates to permanently fail with -EINVAL until
the driver is reloaded.
[ ... ]
> +static int aiu_encoder_i2s_trigger(struct snd_pcm_substream *substream,
> + int cmd,
> + struct snd_soc_dai *dai)
> +{
> + struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
> + int ret;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = gx_stream_start(ts);
> + break;
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + case SNDRV_PCM_TRIGGER_STOP:
> + gx_stream_stop(ts);
> + ret = 0;
> + break;
The PCM trigger callback typically executes in an atomic context with the
PCM stream spinlock held and hardware interrupts disabled, unless the DAI
is explicitly marked with nonatomic = 1.
Looking at gx_stream_start() and gx_stream_stop(), they both unconditionally
acquire a sleeping lock via mutex_lock(&ts->lock).
Does acquiring a mutex in this atomic context trigger a scheduling while
atomic kernel panic?
[ ... ]
> +static int aiu_encoder_i2s_remove_dai(struct snd_soc_dai *dai)
> +{
> + int stream;
> +
> + for_each_pcm_streams(stream) {
> + struct gx_stream *ts = snd_soc_dai_dma_data_get(dai, stream);
> +
> + if (ts)
> + gx_stream_free(ts);
> + }
> +
> + return 0;
> +}
> +
> +static int aiu_encoder_i2s_probe_dai(struct snd_soc_dai *dai)
> +{
> + struct aiu *aiu = snd_soc_dai_get_drvdata(dai);
> + struct gx_iface *iface = &aiu->i2s.iface;
> + int stream;
> +
> + for_each_pcm_streams(stream) {
> + struct gx_stream *ts;
> +
> + if (!snd_soc_dai_get_widget(dai, stream))
> + continue;
> +
> + ts = gx_stream_alloc(iface);
> + if (!ts) {
> + aiu_encoder_i2s_remove_dai(dai);
> + return -ENOMEM;
> + }
> + snd_soc_dai_dma_data_set(dai, stream, ts);
> + }
If gx_stream_alloc() fails for the second stream in
aiu_encoder_i2s_probe_dai(), it calls aiu_encoder_i2s_remove_dai() to clean
up the already allocated streams.
Since aiu_encoder_i2s_remove_dai() retrieves the gx_stream and frees it, but
does not clear the pointer via snd_soc_dai_dma_data_set(dai, stream, NULL),
the snd_soc_dai structure will still contain a dangling pointer.
If a subsequent deferred probe retry fails allocation again, will
remove_dai() fetch the same dangling pointer and pass it to gx_stream_free()
a second time, resulting in a double-free memory corruption?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com?part=2
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/4] ASoC: meson: aiu: introduce I2S output formatter
2026-05-15 15:10 [PATCH 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
2026-05-15 15:10 ` [PATCH 1/4] ASoC: meson: gx: add gx-formatter and gx-interface Valerio Setti
2026-05-15 15:10 ` [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures Valerio Setti
@ 2026-05-15 15:10 ` Valerio Setti
2026-05-15 15:10 ` [PATCH 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
3 siblings, 0 replies; 8+ messages in thread
From: Valerio Setti @ 2026-05-15 15:10 UTC (permalink / raw)
To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
Cc: linux-kernel, linux-sound, linux-arm-kernel, linux-amlogic,
Valerio Setti
Introduce aiu-formatter-i2s, a gx_formatter implementation for the AIU I2S
playback path. This is going to replace data formatting tasks that are
currently being implemented in aiu-encoder-i2s.
This should ideally follow the same design pattern used on the AXG
platform (see axg-tdmout), but the problem here is that all playback
features (including data formatting) so far are implemented in the AIU
component. Getting the full AXG design would mean introducing incompatible
device-tree changes. Therefore aiu-formatter-i2s is kept very simple and
it only implements the bare minimum functionalities to provide I2S
playback formatting. It's not a standalone component though because this
is still implemented by AIU.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
sound/soc/meson/Makefile | 1 +
sound/soc/meson/aiu-formatter-i2s.c | 106 ++++++++++++++++++++++++++++++++++++
2 files changed, 107 insertions(+)
diff --git a/sound/soc/meson/Makefile b/sound/soc/meson/Makefile
index 146ec81526ba091a174a113ce3d8412ddbbfd9dd..f9ec0ebb01f048728b8f85fd8e58fb90df990470 100644
--- a/sound/soc/meson/Makefile
+++ b/sound/soc/meson/Makefile
@@ -5,6 +5,7 @@ snd-soc-meson-aiu-y += aiu-acodec-ctrl.o
snd-soc-meson-aiu-y += aiu-codec-ctrl.o
snd-soc-meson-aiu-y += aiu-encoder-i2s.o
snd-soc-meson-aiu-y += gx-formatter.o
+snd-soc-meson-aiu-y += aiu-formatter-i2s.o
snd-soc-meson-aiu-y += aiu-encoder-spdif.o
snd-soc-meson-aiu-y += aiu-fifo.o
snd-soc-meson-aiu-y += aiu-fifo-i2s.o
diff --git a/sound/soc/meson/aiu-formatter-i2s.c b/sound/soc/meson/aiu-formatter-i2s.c
new file mode 100644
index 0000000000000000000000000000000000000000..c7eff04521de3c282f7f79864143e073ff1b2f27
--- /dev/null
+++ b/sound/soc/meson/aiu-formatter-i2s.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2026 BayLibre, SAS.
+// Author: Valerio Setti <vsetti@baylibre.com>
+
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/soc-dai.h>
+
+#include "aiu.h"
+#include "gx-formatter.h"
+
+#define AIU_I2S_SOURCE_DESC_MODE_8CH BIT(0)
+#define AIU_I2S_SOURCE_DESC_MODE_24BIT BIT(5)
+#define AIU_I2S_SOURCE_DESC_MODE_32BIT BIT(9)
+#define AIU_I2S_SOURCE_DESC_MODE_SPLIT BIT(11)
+#define AIU_RST_SOFT_I2S_FAST BIT(0)
+
+#define AIU_I2S_DAC_CFG_MSB_FIRST BIT(2)
+
+static struct snd_soc_dai *
+aiu_formatter_i2s_get_be(struct snd_soc_dapm_widget *w)
+{
+ struct snd_soc_dapm_path *p;
+ struct snd_soc_dai *be;
+
+ snd_soc_dapm_widget_for_each_sink_path(w, p) {
+ if (!p->connect)
+ continue;
+
+ if (p->sink->id == snd_soc_dapm_dai_in)
+ return (struct snd_soc_dai *)p->sink->priv;
+
+ be = aiu_formatter_i2s_get_be(p->sink);
+ if (be)
+ return be;
+ }
+
+ return NULL;
+}
+
+static struct gx_stream *
+aiu_formatter_i2s_get_stream(struct snd_soc_dapm_widget *w)
+{
+ struct snd_soc_dai *be = aiu_formatter_i2s_get_be(w);
+
+ if (!be)
+ return NULL;
+
+ return snd_soc_dai_dma_data_get_playback(be);
+}
+
+static int aiu_formatter_i2s_prepare(struct regmap *map,
+ const struct gx_formatter_hw *quirks,
+ struct gx_stream *ts)
+{
+ /* Always operate in split (classic interleaved) mode */
+ unsigned int desc = AIU_I2S_SOURCE_DESC_MODE_SPLIT;
+ unsigned int tmp;
+
+ /* Reset required to update the pipeline */
+ regmap_write(map, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
+ regmap_read(map, AIU_I2S_SYNC, &tmp);
+
+ switch (ts->physical_width) {
+ case 16: /* Nothing to do */
+ break;
+
+ case 32:
+ desc |= (AIU_I2S_SOURCE_DESC_MODE_24BIT |
+ AIU_I2S_SOURCE_DESC_MODE_32BIT);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ switch (ts->channels) {
+ case 2: /* Nothing to do */
+ break;
+ case 8:
+ desc |= AIU_I2S_SOURCE_DESC_MODE_8CH;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ regmap_update_bits(map, AIU_I2S_SOURCE_DESC,
+ AIU_I2S_SOURCE_DESC_MODE_8CH |
+ AIU_I2S_SOURCE_DESC_MODE_24BIT |
+ AIU_I2S_SOURCE_DESC_MODE_32BIT |
+ AIU_I2S_SOURCE_DESC_MODE_SPLIT,
+ desc);
+
+ /* Send data MSB first */
+ regmap_update_bits(map, AIU_I2S_DAC_CFG,
+ AIU_I2S_DAC_CFG_MSB_FIRST,
+ AIU_I2S_DAC_CFG_MSB_FIRST);
+
+ return 0;
+}
+
+const struct gx_formatter_ops aiu_formatter_i2s_ops = {
+ .get_stream = aiu_formatter_i2s_get_stream,
+ .prepare = aiu_formatter_i2s_prepare,
+};
--
2.39.5
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
2026-05-15 15:10 [PATCH 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
` (2 preceding siblings ...)
2026-05-15 15:10 ` [PATCH 3/4] ASoC: meson: aiu: introduce I2S output formatter Valerio Setti
@ 2026-05-15 15:10 ` Valerio Setti
2026-05-15 17:40 ` sashiko-bot
3 siblings, 1 reply; 8+ messages in thread
From: Valerio Setti @ 2026-05-15 15:10 UTC (permalink / raw)
To: Jerome Brunet, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Neil Armstrong, Kevin Hilman, Martin Blumenstingl
Cc: linux-kernel, linux-sound, linux-arm-kernel, linux-amlogic,
Valerio Setti
Create a new DAPM widget for "I2S formatter" and place it on the path
between FIFO and output DAI interface. Remove I2S output formatting code
from aiu-encoder-i2s since it's now implemented from aiu-formatter-i2s.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
sound/soc/meson/aiu-encoder-i2s.c | 56 ---------------------------------------
sound/soc/meson/aiu.c | 30 ++++++++++++++++++---
sound/soc/meson/aiu.h | 1 +
3 files changed, 27 insertions(+), 60 deletions(-)
diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index 39accd396affb8beb49fa7cca394244730b24574..9f935a93aeab7a27f880fbde2d29041a4802e3a3 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -13,13 +13,6 @@
#include "gx-formatter.h"
#include "gx-interface.h"
-#define AIU_I2S_SOURCE_DESC_MODE_8CH BIT(0)
-#define AIU_I2S_SOURCE_DESC_MODE_24BIT BIT(5)
-#define AIU_I2S_SOURCE_DESC_MODE_32BIT BIT(9)
-#define AIU_I2S_SOURCE_DESC_MODE_SPLIT BIT(11)
-#define AIU_RST_SOFT_I2S_FAST BIT(0)
-
-#define AIU_I2S_DAC_CFG_MSB_FIRST BIT(2)
#define AIU_CLK_CTRL_I2S_DIV_EN BIT(0)
#define AIU_CLK_CTRL_I2S_DIV GENMASK(3, 2)
#define AIU_CLK_CTRL_AOCLK_INVERT BIT(6)
@@ -37,49 +30,6 @@ static void aiu_encoder_i2s_divider_enable(struct snd_soc_component *component,
enable ? AIU_CLK_CTRL_I2S_DIV_EN : 0);
}
-static int aiu_encoder_i2s_setup_desc(struct snd_soc_component *component,
- struct snd_pcm_hw_params *params)
-{
- /* Always operate in split (classic interleaved) mode */
- unsigned int desc = AIU_I2S_SOURCE_DESC_MODE_SPLIT;
-
- /* Reset required to update the pipeline */
- snd_soc_component_write(component, AIU_RST_SOFT, AIU_RST_SOFT_I2S_FAST);
- snd_soc_component_read(component, AIU_I2S_SYNC);
-
- switch (params_physical_width(params)) {
- case 16: /* Nothing to do */
- break;
-
- case 32:
- desc |= (AIU_I2S_SOURCE_DESC_MODE_24BIT |
- AIU_I2S_SOURCE_DESC_MODE_32BIT);
- break;
-
- default:
- return -EINVAL;
- }
-
- switch (params_channels(params)) {
- case 2: /* Nothing to do */
- break;
- case 8:
- desc |= AIU_I2S_SOURCE_DESC_MODE_8CH;
- break;
- default:
- return -EINVAL;
- }
-
- snd_soc_component_update_bits(component, AIU_I2S_SOURCE_DESC,
- AIU_I2S_SOURCE_DESC_MODE_8CH |
- AIU_I2S_SOURCE_DESC_MODE_24BIT |
- AIU_I2S_SOURCE_DESC_MODE_32BIT |
- AIU_I2S_SOURCE_DESC_MODE_SPLIT,
- desc);
-
- return 0;
-}
-
static int aiu_encoder_i2s_set_legacy_div(struct snd_soc_component *component,
struct gx_stream *ts,
unsigned int bs)
@@ -201,12 +151,6 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
ts->width = params_width(params);
ts->channels = params_channels(params);
- ret = aiu_encoder_i2s_setup_desc(component, params);
- if (ret) {
- dev_err(dai->dev, "setting i2s desc failed: %d\n", ret);
- return ret;
- }
-
ret = aiu_encoder_i2s_set_clocks(component, ts);
if (ret) {
dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
index f2890111c1d2cfa2213bf01849957a796744b9ae..b09c2058eacaf2998d0d3cd6682910f94ec89912 100644
--- a/sound/soc/meson/aiu.c
+++ b/sound/soc/meson/aiu.c
@@ -29,13 +29,22 @@ static SOC_ENUM_SINGLE_DECL(aiu_spdif_encode_sel_enum, AIU_I2S_MISC,
static const struct snd_kcontrol_new aiu_spdif_encode_mux =
SOC_DAPM_ENUM("SPDIF Buffer Src", aiu_spdif_encode_sel_enum);
-static const struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {
- SND_SOC_DAPM_MUX("SPDIF SRC SEL", SND_SOC_NOPM, 0, 0,
- &aiu_spdif_encode_mux),
+#define AIU_WIDGET_SPDIF_SRC_SEL 0
+#define AIU_WIDGET_I2S_FORMATTER 1
+
+static struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {
+ [AIU_WIDGET_SPDIF_SRC_SEL] =
+ SND_SOC_DAPM_MUX("SPDIF SRC SEL", SND_SOC_NOPM, 0, 0,
+ &aiu_spdif_encode_mux),
+ [AIU_WIDGET_I2S_FORMATTER] =
+ SND_SOC_DAPM_PGA_E("I2S Formatter", SND_SOC_NOPM, 0, 0, NULL, 0,
+ gx_formatter_event,
+ (SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD)),
};
static const struct snd_soc_dapm_route aiu_cpu_dapm_routes[] = {
- { "I2S Encoder Playback", NULL, "I2S FIFO Playback" },
+ { "I2S Formatter", NULL, "I2S FIFO Playback" },
+ { "I2S Encoder Playback", NULL, "I2S Formatter" },
{ "SPDIF SRC SEL", "SPDIF", "SPDIF FIFO Playback" },
{ "SPDIF SRC SEL", "I2S", "I2S FIFO Playback" },
{ "SPDIF Encoder Playback", NULL, "SPDIF SRC SEL" },
@@ -172,6 +181,11 @@ static const struct regmap_config aiu_regmap_cfg = {
.max_register = 0x2ac,
};
+const struct gx_formatter_driver aiu_formatter_i2s_drv = {
+ .regmap_cfg = &aiu_regmap_cfg,
+ .ops = &aiu_formatter_i2s_ops,
+};
+
static int aiu_clk_bulk_get(struct device *dev,
const char * const *ids,
unsigned int num,
@@ -291,6 +305,14 @@ static int aiu_probe(struct platform_device *pdev)
return ret;
}
+ /* Allocate the aiu-formatter into its widget */
+ ret = gx_formatter_create(dev, &aiu_cpu_dapm_widgets[AIU_WIDGET_I2S_FORMATTER],
+ &aiu_formatter_i2s_drv, map);
+ if (ret) {
+ dev_err(dev, "Failed to allocate aiu formatter\n");
+ goto err;
+ }
+
/* Register the hdmi codec control component */
ret = aiu_hdmi_ctrl_register_component(dev);
if (ret) {
diff --git a/sound/soc/meson/aiu.h b/sound/soc/meson/aiu.h
index 68310de0bdf7a97d8de2ff306c159248ee9b0ede..7d0b98c1f351b3c526ca06c43a4c04ee5f4b6dfa 100644
--- a/sound/soc/meson/aiu.h
+++ b/sound/soc/meson/aiu.h
@@ -61,6 +61,7 @@ extern const struct snd_soc_dai_ops aiu_fifo_i2s_dai_ops;
extern const struct snd_soc_dai_ops aiu_fifo_spdif_dai_ops;
extern const struct snd_soc_dai_ops aiu_encoder_i2s_dai_ops;
extern const struct snd_soc_dai_ops aiu_encoder_spdif_dai_ops;
+extern const struct gx_formatter_ops aiu_formatter_i2s_ops;
#define AIU_IEC958_BPF 0x000
#define AIU_958_MISC 0x010
--
2.39.5
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
2026-05-15 15:10 ` [PATCH 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
@ 2026-05-15 17:40 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-05-15 17:40 UTC (permalink / raw)
To: Valerio Setti; +Cc: linux-amlogic, neil.armstrong
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Modifying a global static DAPM widget template with a devm-managed pointer creates a Use-After-Free vulnerability.
- [High] Initialization order flaw between `snd_soc_register_component()` and `gx_formatter_create()` causes a type confusion kernel panic.
--
commit a12ba200fe5e3358b227094679f8a17e9ac53591
Author: Valerio Setti <vsetti@baylibre.com>
ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
This commit introduces a new DAPM widget for the I2S formatter and routes
it between the FIFO and output DAI interface. It also removes the existing
I2S output formatting code from aiu-encoder-i2s.
> diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
> index f2890111c1d2c..b09c2058eacaf 100644
> --- a/sound/soc/meson/aiu.c
> +++ b/sound/soc/meson/aiu.c
[ ... ]
> @@ -29,13 +29,22 @@ static SOC_ENUM_SINGLE_DECL(aiu_spdif_encode_sel_enum, AIU_I2S_MISC,
> static const struct snd_kcontrol_new aiu_spdif_encode_mux =
> SOC_DAPM_ENUM("SPDIF Buffer Src", aiu_spdif_encode_sel_enum);
>
> -static const struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {
> - SND_SOC_DAPM_MUX("SPDIF SRC SEL", SND_SOC_NOPM, 0, 0,
> - &aiu_spdif_encode_mux),
> +#define AIU_WIDGET_SPDIF_SRC_SEL 0
> +#define AIU_WIDGET_I2S_FORMATTER 1
> +
> +static struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {
Is it safe to modify this global static template with a devm-managed
pointer later in aiu_probe?
If a subsequent initialization step fails and aiu_probe returns an error,
the devres subsystem will automatically free the allocated memory. However,
the dangling pointer will remain in this global static template.
Upon a subsequent probe deferral, will snd_soc_register_component clone
this corrupted template, bringing the freed pointer into the active DAPM
context and causing a use-after-free when gx_formatter_event dereferences
w->priv?
[ ... ]
> @@ -291,6 +305,14 @@ static int aiu_probe(struct platform_device *pdev)
> return ret;
> }
>
> + /* Allocate the aiu-formatter into its widget */
> + ret = gx_formatter_create(dev, &aiu_cpu_dapm_widgets[AIU_WIDGET_I2S_FORMATTER],
> + &aiu_formatter_i2s_drv, map);
Could this initialization order cause a type confusion panic?
snd_soc_register_component is invoked right before this in aiu_probe.
If ASoC triggers synchronous card binding upon component registration, the
DAPM widgets are cloned from the template while w->priv is still NULL.
Later, during audio playback, if gx_formatter_event runs, it checks
w->priv, finds it NULL, and executes the fallback path using
snd_soc_component_get_drvdata.
The component drvdata for the AIU CPU component is explicitly set to
struct aiu * via platform_set_drvdata earlier in the probe, not a
struct gx_formatter *.
Will the code erroneously treat the struct aiu * as a struct gx_formatter *
and attempt to dereference its fields, resulting in a panic?
> + if (ret) {
> + dev_err(dev, "Failed to allocate aiu formatter\n");
> + goto err;
> + }
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com?part=4
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 8+ messages in thread