* [PATCH v2 1/4] ASoC: meson: gx: add gx-formatter and gx-interface
2026-06-10 21:29 [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
@ 2026-06-10 21:29 ` Valerio Setti
2026-06-10 21:29 ` [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams Valerio Setti
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Valerio Setti @ 2026-06-10 21:29 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 | 282 +++++++++++++++++++++++++++++++++++++++++
sound/soc/meson/gx-formatter.h | 56 ++++++++
sound/soc/meson/gx-interface.h | 48 +++++++
4 files changed, 387 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..311e63affb239ee575d59b7c6ea5c1c5f3ab2300
--- /dev/null
+++ b/sound/soc/meson/gx-formatter.c
@@ -0,0 +1,282 @@
+// 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;
+
+ if (!ts)
+ return;
+
+ 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;
+ INIT_LIST_HEAD(&formatter->list);
+ 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;
+}
+EXPORT_SYMBOL_GPL(gx_formatter_create);
+
+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 enable formatter\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..b90b1814d79b49e3e6e5f4470161bc1e8bba6ebd
--- /dev/null
+++ b/sound/soc/meson/gx-formatter.h
@@ -0,0 +1,56 @@
+/* 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);
+
+/*
+ * Formatter data is already freed when the associated device is removed,
+ * so we just need to remove the pointer from the widget.
+ */
+static inline void gx_formatter_free(struct snd_soc_dapm_widget *w)
+{
+ w->priv = NULL;
+}
+
+#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..65c46dcce32a8c2d2a95afe3b99e65e759781a6a
--- /dev/null
+++ b/sound/soc/meson/gx-interface.h
@@ -0,0 +1,48 @@
+/* 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;
+
+ /* Only for GX platform */
+ int bs_quirk;
+};
+
+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] 10+ messages in thread* [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
2026-06-10 21:29 [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
2026-06-10 21:29 ` [PATCH v2 1/4] ASoC: meson: gx: add gx-formatter and gx-interface Valerio Setti
@ 2026-06-10 21:29 ` Valerio Setti
2026-06-10 21:41 ` sashiko-bot
2026-06-11 8:16 ` Jerome Brunet
2026-06-10 21:29 ` [PATCH v2 3/4] ASoC: meson: aiu: introduce I2S output formatter Valerio Setti
` (2 subsequent siblings)
4 siblings, 2 replies; 10+ messages in thread
From: Valerio Setti @ 2026-06-10 21:29 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
aiu-encoder-i2s is going to be the interface that handles both playback
and capture, so this commit does all the required changes to prepare
for that since so far it only handled playback:
- probe/remove functions are added to allocate/free per stream data,
respectively.
- 'struc gx_iface' and 'struct gx_stream' are used to store interface or
stream associated data, respecively.
- interface wide rate symmetry is enforced.
- quirks on bclk are also enforced if/when necessary.
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
prepare/hw_free.
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 | 207 ++++++++++++++++++++++++++++++++++----
sound/soc/meson/aiu.h | 3 +
2 files changed, 193 insertions(+), 17 deletions(-)
diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index 3b4061508c18047fe8d6f3f98061720f8ce238f2..f50b03824ad280afabb31eecc20ccb855defa11e 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)
@@ -112,6 +114,9 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
struct snd_pcm_hw_params *params,
unsigned int bs)
{
+ struct aiu *aiu = snd_soc_component_get_drvdata(component);
+ struct gx_iface *iface = &aiu->i2s.iface;
+
/*
* NOTE: this HW is odd.
* In most configuration, the i2s divider is 'mclk / blck'.
@@ -126,6 +131,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
return -EINVAL;
}
bs += bs / 2;
+ iface->bs_quirk = true;
+ } else {
+ /*
+ * If the bs quirk is currently applied for one stream and another
+ * ones tries to setup a configuration for which the quirk is
+ * not required, then fail.
+ */
+ if (iface->bs_quirk) {
+ dev_err(component->dev,
+ "bclk requirements are incompatible with active stream\n");
+ return -EINVAL;
+ }
}
/* Use CLK_MORE for mclk to bclk divider */
@@ -145,14 +162,15 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
struct snd_pcm_hw_params *params)
{
struct aiu *aiu = snd_soc_component_get_drvdata(component);
+ struct gx_iface *iface = &aiu->i2s.iface;
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(iface->mclk_rate, srate);
- if (fs % 64)
+ if ((fs % 64) || (fs == 0))
return -EINVAL;
/* Send data MSB first */
@@ -188,24 +206,59 @@ 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 only if there is more than
+ * 1 stream active.
+ */
+ if (snd_soc_dai_active(dai) > 1) {
+ 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;
+ }
+ }
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);
if (ret) {
- dev_err(dai->dev, "setting i2s clocks failed\n");
+ dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
return ret;
}
+ iface->rate = params_rate(params);
+ ts->physical_width = params_physical_width(params);
+ ts->width = params_width(params);
+ ts->channels = params_channels(params);
+
+ 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, true);
return 0;
@@ -214,9 +267,24 @@ 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 gx_iface *iface = ts->iface;
struct snd_soc_component *component = dai->component;
- aiu_encoder_i2s_divider_enable(component, false);
+ /*
+ * If this is the last substream being closed then disable the i2s
+ * clock divider and clear 'iface->rate'.
+ */
+ if (snd_soc_dai_active(dai) <= 1) {
+ aiu_encoder_i2s_divider_enable(component, 0);
+ iface->rate = 0;
+ iface->bs_quirk = false;
+ }
+
+ if (ts->clk_enabled) {
+ clk_disable_unprepare(ts->iface->mclk);
+ ts->clk_enabled = false;
+ }
return 0;
}
@@ -224,6 +292,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 +325,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 |
@@ -272,6 +345,7 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int freq, int dir)
{
struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
+ struct gx_iface *iface = &aiu->i2s.iface;
int ret;
if (WARN_ON(clk_id != 0))
@@ -280,11 +354,15 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
if (dir == SND_SOC_CLOCK_IN)
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);
+ ret = clk_set_rate(iface->mclk, freq);
+ if (ret) {
+ dev_err(dai->dev, "Failed to set sysclk to %uHz: %d", freq, ret);
+ return ret;
+ }
- return ret;
+ iface->mclk_rate = freq;
+
+ return 0;
}
static const unsigned int hw_channels[] = {2, 8};
@@ -305,15 +383,35 @@ 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,
@@ -321,14 +419,89 @@ static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
{
struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
- clk_bulk_disable_unprepare(aiu->i2s.clk_num, aiu->i2s.clks);
+ clk_disable_unprepare(aiu->i2s.clks[AOCLK].clk);
+ clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
+ clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
+}
+
+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;
+
+ ts = snd_soc_dai_dma_data_get(dai, stream);
+ if (ts)
+ gx_stream_free(ts);
+
+ snd_soc_dai_dma_data_set(dai, stream, NULL);
+ }
+
+ 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;
+ iface->mclk_rate = clk_get_rate(iface->mclk);
+
+ 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] 10+ messages in thread* Re: [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
2026-06-10 21:29 ` [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams Valerio Setti
@ 2026-06-10 21:41 ` sashiko-bot
2026-06-11 8:16 ` Jerome Brunet
1 sibling, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 21:41 UTC (permalink / raw)
To: Valerio Setti; +Cc: neil.armstrong, linux-amlogic
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asymmetric `bs_quirk` validation allows a new stream to corrupt the BCLK divider of an already active non-quirk stream.
- [Medium] Improper state tracking using `snd_soc_dai_active(dai)` in `hw_free` leaks shared interface state, locking the DAI's parameters.
--
commit b82d5ce50c8fb1a7cfc960367388f0f096fbaa05
Author: Valerio Setti <vsetti@baylibre.com>
ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
This commit prepares the aiu-encoder-i2s interface to handle both playback
and capture streams. It introduces per-stream data allocation, enforces
interface-wide rate symmetry, and modifies clock handling to enable
internal clocks on startup while deferring MCLK and I2S clock dividers to
prepare/hw_free.
> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c180..f50b03824ad28 100644
> --- a/sound/soc/meson/aiu-encoder-i2s.c
> +++ b/sound/soc/meson/aiu-encoder-i2s.c
[ ... ]
> @@ -126,6 +131,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> return -EINVAL;
> }
> bs += bs / 2;
> + iface->bs_quirk = true;
> + } else {
[Severity: High]
Does this branch allow a new stream to corrupt the clock divider of an
already active non-quirk stream?
The else branch correctly prevents a new non-quirk stream from configuring
if iface->bs_quirk is already true. However, the if branch seems to miss
the reverse check.
If a normal stream (like 2-channel audio) is already active, iface->bs_quirk
remains false. If a second stream is then configured with 16-bit 8-channels,
it appears this code will calculate the 50% increase, set
iface->bs_quirk = true, and unconditionally overwrite the shared register,
which might corrupt the bitclock frequency of the already running first
stream.
> + /*
> + * If the bs quirk is currently applied for one stream and another
> + * ones tries to setup a configuration for which the quirk is
> + * not required, then fail.
> + */
> + if (iface->bs_quirk) {
> + dev_err(component->dev,
> + "bclk requirements are incompatible with active stream\n");
> + return -EINVAL;
> + }
> }
[ ... ]
> @@ -188,24 +206,59 @@ 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 only if there is more than
> + * 1 stream active.
> + */
> + if (snd_soc_dai_active(dai) > 1) {
> + 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;
> + }
> + }
[ ... ]
> @@ -214,9 +267,24 @@ 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 gx_iface *iface = ts->iface;
> struct snd_soc_component *component = dai->component;
>
> - aiu_encoder_i2s_divider_enable(component, false);
> + /*
> + * If this is the last substream being closed then disable the i2s
> + * clock divider and clear 'iface->rate'.
> + */
> + if (snd_soc_dai_active(dai) <= 1) {
> + aiu_encoder_i2s_divider_enable(component, 0);
> + iface->rate = 0;
> + iface->bs_quirk = false;
> + }
[Severity: Medium]
Can this leak the shared interface state and lock the DAI's parameters?
snd_soc_dai_active() tracks the number of open file descriptors, not the
number of streams with active hardware parameters.
If an application opens both playback and capture (making active == 2),
configures the playback stream, and then frees the hardware parameters via
SNDRV_PCM_IOCTL_HW_FREE without closing the file descriptors, this hw_free
callback will see active == 2.
Could this cause iface->rate to be leaked into a zombie state, preventing
valid format reconfigurations without fully closing the devices, and causing
subsequent hw_params attempts to fail the symmetry check above?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-reshape-aiu-as-axg-v2-0-cac3663a8b51@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] 10+ messages in thread* Re: [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
2026-06-10 21:29 ` [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams Valerio Setti
2026-06-10 21:41 ` sashiko-bot
@ 2026-06-11 8:16 ` Jerome Brunet
2026-06-12 13:19 ` Mark Brown
1 sibling, 1 reply; 10+ messages in thread
From: Jerome Brunet @ 2026-06-11 8:16 UTC (permalink / raw)
To: Valerio Setti
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-kernel,
linux-sound, linux-arm-kernel, linux-amlogic
On mer. 10 juin 2026 at 23:29, Valerio Setti <vsetti@baylibre.com> wrote:
> aiu-encoder-i2s is going to be the interface that handles both playback
> and capture, so this commit does all the required changes to prepare
> for that since so far it only handled playback:
> - probe/remove functions are added to allocate/free per stream data,
> respectively.
> - 'struc gx_iface' and 'struct gx_stream' are used to store interface or
> stream associated data, respecively.
> - interface wide rate symmetry is enforced.
> - quirks on bclk are also enforced if/when necessary.
>
> 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
> prepare/hw_free.
>
> 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 | 207 ++++++++++++++++++++++++++++++++++----
> sound/soc/meson/aiu.h | 3 +
> 2 files changed, 193 insertions(+), 17 deletions(-)
>
> diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
> index 3b4061508c18047fe8d6f3f98061720f8ce238f2..f50b03824ad280afabb31eecc20ccb855defa11e 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)
> @@ -112,6 +114,9 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> struct snd_pcm_hw_params *params,
> unsigned int bs)
> {
> + struct aiu *aiu = snd_soc_component_get_drvdata(component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> +
> /*
> * NOTE: this HW is odd.
> * In most configuration, the i2s divider is 'mclk / blck'.
> @@ -126,6 +131,18 @@ static int aiu_encoder_i2s_set_more_div(struct snd_soc_component *component,
> return -EINVAL;
> }
> bs += bs / 2;
> + iface->bs_quirk = true;
> + } else {
> + /*
> + * If the bs quirk is currently applied for one stream and another
> + * ones tries to setup a configuration for which the quirk is
> + * not required, then fail.
> + */
> + if (iface->bs_quirk) {
> + dev_err(component->dev,
> + "bclk requirements are incompatible with active stream\n");
> + return -EINVAL;
This comment is not blocking thing IMO, more a request for future
improvments.
What I do not like here is the user may get an unexpected failure while
setting up the stream, because of something that did not show up the
contrainst we provided. IOW, it is kind of too late.
The problem is not new to this driver, the same applies to the error
returned if bs is not a multiple of 2 :(
It would be nice if this problem could be reflected in the contraints
to avoid returning an error this late in the stream setup.
> + }
> }
>
> /* Use CLK_MORE for mclk to bclk divider */
> @@ -145,14 +162,15 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
> struct snd_pcm_hw_params *params)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> 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(iface->mclk_rate, srate);
>
> - if (fs % 64)
> + if ((fs % 64) || (fs == 0))
> return -EINVAL;
>
> /* Send data MSB first */
> @@ -188,24 +206,59 @@ 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 only if there is more than
> + * 1 stream active.
> + */
> + if (snd_soc_dai_active(dai) > 1) {
> + 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;
> + }
> + }
>
> 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);
> if (ret) {
> - dev_err(dai->dev, "setting i2s clocks failed\n");
> + dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
> return ret;
> }
>
> + iface->rate = params_rate(params);
> + ts->physical_width = params_physical_width(params);
> + ts->width = params_width(params);
> + ts->channels = params_channels(params);
> +
> + 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, true);
>
> return 0;
> @@ -214,9 +267,24 @@ 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 gx_iface *iface = ts->iface;
> struct snd_soc_component *component = dai->component;
>
> - aiu_encoder_i2s_divider_enable(component, false);
> + /*
> + * If this is the last substream being closed then disable the i2s
> + * clock divider and clear 'iface->rate'.
> + */
> + if (snd_soc_dai_active(dai) <= 1) {
> + aiu_encoder_i2s_divider_enable(component, 0);
> + iface->rate = 0;
> + iface->bs_quirk = false;
> + }
> +
> + if (ts->clk_enabled) {
> + clk_disable_unprepare(ts->iface->mclk);
> + ts->clk_enabled = false;
> + }
>
> return 0;
> }
> @@ -224,6 +292,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 +325,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 |
> @@ -272,6 +345,7 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> unsigned int freq, int dir)
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
> + struct gx_iface *iface = &aiu->i2s.iface;
> int ret;
>
> if (WARN_ON(clk_id != 0))
> @@ -280,11 +354,15 @@ static int aiu_encoder_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> if (dir == SND_SOC_CLOCK_IN)
> 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);
> + ret = clk_set_rate(iface->mclk, freq);
> + if (ret) {
> + dev_err(dai->dev, "Failed to set sysclk to %uHz: %d", freq, ret);
> + return ret;
> + }
>
> - return ret;
> + iface->mclk_rate = freq;
> +
> + return 0;
> }
>
> static const unsigned int hw_channels[] = {2, 8};
> @@ -305,15 +383,35 @@ 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,
> @@ -321,14 +419,89 @@ static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
> {
> struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
>
> - clk_bulk_disable_unprepare(aiu->i2s.clk_num, aiu->i2s.clks);
> + clk_disable_unprepare(aiu->i2s.clks[AOCLK].clk);
> + clk_disable_unprepare(aiu->i2s.clks[MIXER].clk);
> + clk_disable_unprepare(aiu->i2s.clks[PCLK].clk);
> +}
> +
> +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;
> +
> + ts = snd_soc_dai_dma_data_get(dai, stream);
> + if (ts)
> + gx_stream_free(ts);
> +
> + snd_soc_dai_dma_data_set(dai, stream, NULL);
> + }
> +
> + 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;
> + iface->mclk_rate = clk_get_rate(iface->mclk);
> +
> + 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 {
--
Jerome
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
2026-06-11 8:16 ` Jerome Brunet
@ 2026-06-12 13:19 ` Mark Brown
0 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2026-06-12 13:19 UTC (permalink / raw)
To: Jerome Brunet
Cc: Valerio Setti, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-kernel,
linux-sound, linux-arm-kernel, linux-amlogic
[-- Attachment #1.1: Type: text/plain, Size: 669 bytes --]
On Thu, Jun 11, 2026 at 10:16:56AM +0200, Jerome Brunet wrote:
> On mer. 10 juin 2026 at 23:29, Valerio Setti <vsetti@baylibre.com> wrote:
>
> > aiu-encoder-i2s is going to be the interface that handles both playback
> > and capture, so this commit does all the required changes to prepare
> > for that since so far it only handled playback:
> > - probe/remove functions are added to allocate/free per stream data,
> > respectively.
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 167 bytes --]
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] ASoC: meson: aiu: introduce I2S output formatter
2026-06-10 21:29 [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
2026-06-10 21:29 ` [PATCH v2 1/4] ASoC: meson: gx: add gx-formatter and gx-interface Valerio Setti
2026-06-10 21:29 ` [PATCH v2 2/4] ASoC: meson: aiu-encoder-i2s: prepare for multiple streams Valerio Setti
@ 2026-06-10 21:29 ` Valerio Setti
2026-06-10 21:29 ` [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
2026-06-11 8:28 ` [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Jerome Brunet
4 siblings, 0 replies; 10+ messages in thread
From: Valerio Setti @ 2026-06-10 21:29 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), where basically the widget/formatter corresponds
to a single audio component. This is not possible in the GX platform though
because all the features are currently implemented in the AIU audio
component and changing that would require backward 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 belongs to AIU.
Signed-off-by: Valerio Setti <vsetti@baylibre.com>
---
sound/soc/meson/Makefile | 1 +
sound/soc/meson/aiu-formatter-i2s.c | 104 ++++++++++++++++++++++++++++++++++++
2 files changed, 105 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..b4604734fe88da2acd6e5c2f9f59e8ecb0a017a5
--- /dev/null
+++ b/sound/soc/meson/aiu-formatter-i2s.c
@@ -0,0 +1,104 @@
+// 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_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 = 0;
+ 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,
+ 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] 10+ messages in thread* [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
2026-06-10 21:29 [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
` (2 preceding siblings ...)
2026-06-10 21:29 ` [PATCH v2 3/4] ASoC: meson: aiu: introduce I2S output formatter Valerio Setti
@ 2026-06-10 21:29 ` Valerio Setti
2026-06-10 21:39 ` sashiko-bot
2026-06-11 8:28 ` [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Jerome Brunet
4 siblings, 1 reply; 10+ messages in thread
From: Valerio Setti @ 2026-06-10 21:29 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 | 78 ++++++++++-----------------------------
sound/soc/meson/aiu.c | 32 ++++++++++++++--
sound/soc/meson/aiu.h | 1 +
3 files changed, 48 insertions(+), 63 deletions(-)
diff --git a/sound/soc/meson/aiu-encoder-i2s.c b/sound/soc/meson/aiu-encoder-i2s.c
index f50b03824ad280afabb31eecc20ccb855defa11e..83b579e98f1c75ce03fea1c6ff4e99b2fedebe0c 100644
--- a/sound/soc/meson/aiu-encoder-i2s.c
+++ b/sound/soc/meson/aiu-encoder-i2s.c
@@ -13,13 +13,8 @@
#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 +32,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 snd_pcm_hw_params *params,
unsigned int bs)
@@ -173,11 +125,6 @@ static int aiu_encoder_i2s_set_clocks(struct snd_soc_component *component,
if ((fs % 64) || (fs == 0))
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,
@@ -223,12 +170,6 @@ static int aiu_encoder_i2s_hw_params(struct snd_pcm_substream *substream,
}
}
- 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, params);
if (ret) {
dev_err(dai->dev, "setting i2s clocks failed: %d\n", ret);
@@ -411,6 +352,25 @@ static int aiu_encoder_i2s_startup(struct snd_pcm_substream *substream,
return ret;
}
+ /*
+ * We're always operating in split mode for the playback stream.
+ *
+ * This setting arguably belong to the 'aiu-formatter', but it's kept
+ * here for backward compatibility reason. At reset the I2S encoder
+ * operates in normal mode which would only support 8ch, but by default
+ * only 2ch are enabled. If a playback stream is started without
+ * changing to split mode, then the I2S encoder doesn't consume audio
+ * samples and the playback fails.
+ * Moving this to 'aiu-formatter' would cause the split mode to be set
+ * only when the formatter is enabled, which doesn't happen at boot as
+ * the default value for "HDMI CTRL SRC" is "DISABLED".
+ */
+ ret = snd_soc_component_update_bits(dai->component, AIU_I2S_SOURCE_DESC,
+ AIU_I2S_SOURCE_DESC_MODE_SPLIT,
+ AIU_I2S_SOURCE_DESC_MODE_SPLIT);
+ if (ret < 0)
+ dev_err(dai->dev, "failed to update AIU_I2S_SOURCE_DESC: %d", ret);
+
return 0;
}
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
index f2890111c1d2cfa2213bf01849957a796744b9ae..64ace4d25d92cbe137066359a839e1b11bf140f8 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,
@@ -282,6 +296,14 @@ static int aiu_probe(struct platform_device *pdev)
if (ret)
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 cpu component of the aiu */
ret = snd_soc_register_component(dev, &aiu_cpu_component,
aiu_cpu_dai_drv,
@@ -310,12 +332,14 @@ static int aiu_probe(struct platform_device *pdev)
return 0;
err:
+ gx_formatter_free(&aiu_cpu_dapm_widgets[AIU_WIDGET_I2S_FORMATTER]);
snd_soc_unregister_component(dev);
return ret;
}
static void aiu_remove(struct platform_device *pdev)
{
+ gx_formatter_free(&aiu_cpu_dapm_widgets[AIU_WIDGET_I2S_FORMATTER]);
snd_soc_unregister_component(&pdev->dev);
}
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] 10+ messages in thread* Re: [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
2026-06-10 21:29 ` [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
@ 2026-06-10 21:39 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-06-10 21:39 UTC (permalink / raw)
To: Valerio Setti; +Cc: neil.armstrong, linux-amlogic
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mutation of a global static template array with instance-specific data breaks the device driver model and enables data races.
- [Medium] Missing cleanup of global template array on error path, causing a dangling pointer.
--
commit 0aa6e4b317dd7227cb1e1e58ed456735772485cc
Author: Valerio Setti <vsetti@baylibre.com>
ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
Create a new DAPM widget for the "I2S formatter" and place it on the path
between the FIFO and output DAI interface. This also removes the I2S output
formatting code from aiu-encoder-i2s since it's now handled by the new
aiu-formatter-i2s widget.
> diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
> index f2890111c1d2c..64ace4d25d92c 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[] = {
[Severity: High]
Does removing the const qualifier here and modifying the global template array
during aiu_probe() break the device driver model?
If multiple instances of this device are probed, won't they overwrite each
other's instance pointers in this shared array, potentially causing data races
or a use-after-free?
> + [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)),
> };
[ ... ]
> @@ -282,6 +296,14 @@ static int aiu_probe(struct platform_device *pdev)
> if (ret)
> 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 cpu component of the aiu */
> ret = snd_soc_register_component(dev, &aiu_cpu_component,
> aiu_cpu_dai_drv,
[Severity: Medium]
If snd_soc_register_component() fails, should this path use "goto err" instead
of returning directly?
Looking at the rest of aiu_probe():
ret = snd_soc_register_component(dev, &aiu_cpu_component,
aiu_cpu_dai_drv,
ARRAY_SIZE(aiu_cpu_dai_drv));
if (ret) {
dev_err(dev, "Failed to register cpu component\n");
return ret;
}
Since gx_formatter_create() allocates memory and assigns it to the global
array, returning directly skips the gx_formatter_free() call at the err label.
The devres framework will free the memory, but wouldn't this leave a dangling
pointer in the global aiu_cpu_dapm_widgets array?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-reshape-aiu-as-axg-v2-0-cac3663a8b51@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] 10+ messages in thread
* Re: [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one
2026-06-10 21:29 [PATCH v2 0/4] ASoC: meson: aiu: align I2S design to the AXG one Valerio Setti
` (3 preceding siblings ...)
2026-06-10 21:29 ` [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data Valerio Setti
@ 2026-06-11 8:28 ` Jerome Brunet
4 siblings, 0 replies; 10+ messages in thread
From: Jerome Brunet @ 2026-06-11 8:28 UTC (permalink / raw)
To: Valerio Setti
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Neil Armstrong, Kevin Hilman, Martin Blumenstingl, linux-kernel,
linux-sound, linux-arm-kernel, linux-amlogic
On mer. 10 juin 2026 at 23:29, Valerio Setti <vsetti@baylibre.com> wrote:
> The goal of this series is to reshape Amlogic GX's AIU implementation for
> I2S to let it follow the same design as in AXG's TDM. Keeping the same
> design allows for unifying the two platform implementations in the future
> and it also allows for an easy addition of I2S input.
>
> The first commit introduces gx-formatter as the basic block which takes
> care of properly formatting audio data. Formatters are DAPM widgets
> (c.f. axg-tdm-formatter in AXG) which are dynamically attached/detached
> to the streams when the latters starts/stop, respectively.
> aiu-formatter-i2s is introduced as formatter implementation for the i2s
> output.
>
> By the end aiu-encoder-i2s will only need to handle interface clocks and
> enforce interface wide rate symmetry (c.f axg-tdm-interface on the AXG
> platform). Right now rate symmetry is not relevant because only i2s output
> is supported, but it will become useful when following patch series will
> introduce the i2s input part.
>
> This series was tested on an OdroidC2 board (Amlogic S905 SOC) both with
> HDMI output and with NXP SGTL5000 codec connected to the I2S pins.
> This series was also verified using "pcm-test" test tool and all tests
> are passing.
>
> Changes in v2:
> - Fixed most of the weaknesses found by Sashiko review tool [1].
> - Resolved testing failures with "pcm-test" as reported by Mark Brown
> (thanks for the heads up!). I left a comment in
> "aiu_encoder_i2s_startup" to explain the fix.
>
> Link to v1: https://lore.kernel.org/r/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3@baylibre.com
>
> [1]: https://sashiko.dev/#/patchset/20260515-reshape-aiu-as-axg-v1-0-53b457784ff3%40baylibre.com
>
> Signed-off-by: Valerio Setti <vsetti@baylibre.com>
Nice work. Thanks for taking the time to understand the existing drivers
and build upon them !
Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
Feel free to add yourself as maintainer of the Amlogic audio drivers, if
you fancy doing so ;)
> ---
> Valerio Setti (4):
> ASoC: meson: gx: add gx-formatter and gx-interface
> ASoC: meson: aiu-encoder-i2s: prepare for multiple streams
> ASoC: meson: aiu: introduce I2S output formatter
> ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data
>
> sound/soc/meson/Makefile | 2 +
> sound/soc/meson/aiu-encoder-i2s.c | 281 +++++++++++++++++++++++++----------
> sound/soc/meson/aiu-formatter-i2s.c | 104 +++++++++++++
> sound/soc/meson/aiu.c | 32 +++-
> sound/soc/meson/aiu.h | 4 +
> sound/soc/meson/gx-formatter.c | 282 ++++++++++++++++++++++++++++++++++++
> sound/soc/meson/gx-formatter.h | 56 +++++++
> sound/soc/meson/gx-interface.h | 48 ++++++
> 8 files changed, 731 insertions(+), 78 deletions(-)
> ---
> base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
> change-id: 20260515-reshape-aiu-as-axg-1dac9037cad3
>
> Best regards,
--
Jerome
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 10+ messages in thread