[RFC PATCH 0/2] media: synopsys: hdmirx: add HDMI audio capture support

[RFC PATCH 0/2] media: synopsys: hdmirx: add HDMI audio capture support

[Igor Paunovic]

This series adds audio capture support to the Synopsys DesignWare HDMI
RX driver, which currently handles video only. The controller already
extracts the audio stream embedded in the incoming HDMI signal and can
feed it to an on-SoC I2S interface.

Patch 1 documents #sound-dai-cells in the binding. Patch 2 registers
the generic hdmi-codec as a child device of the controller so that a
simple-audio-card in the device tree can bind the HDMI RX audio DAI.
The audio sample rate is recovered from the ACR N/CTS values together
with the measured TMDS character rate, and a periodic worker keeps the
local audio reference clock locked to the source by nudging it in small
ppm steps to hold the audio FIFO fill level near its target, avoiding
FIFO under/overflow and dropped samples.

Tested on an Orange Pi 5 Plus (RK3588, i2s7_8ch as the capture DAI)
with several HDMI sources: stable capture at 44.1 kHz and 48 kHz,
sample rate switching, long recordings with no drift-related drops, and
live A/V monitoring alongside the existing video capture.

Sent as RFC to ask whether this is the preferred shape for the RX
driver: the hdmi-codec child + DT sound card plumbing follows the
pattern used on the TX side by dw-hdmi. The board-side DT changes
(enabling the I2S interface and adding the sound card) are not part of
this series and would go through the SoC tree separately.

Igor Paunovic (2):
  dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
  media: synopsys: hdmirx: add HDMI audio capture support

 .../bindings/media/snps,dw-hdmi-rx.yaml       |   7 +
 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 244 ++++++++++++++++++
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |   8 +
 3 files changed, 259 insertions(+)

--
2.53.0

[RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Igor Paunovic]

The HDMI RX controller can expose the audio embedded in the incoming
HDMI stream as an ALSA capture device. Document the #sound-dai-cells
property so that a sound card can reference the HDMI RX audio DAI.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
 .../devicetree/bindings/media/snps,dw-hdmi-rx.yaml         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
index b7f6c87..83c10d9 100644
--- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
+++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
@@ -78,6 +78,13 @@ properties:
       The phandle of the syscon node for the Video Output GRF register
       to enable EDID transfer through SDAIN and SCLIN.
 
+  "#sound-dai-cells":
+    const: 1
+    description:
+      The HDMI RX controller registers a generic HDMI audio codec so the
+      audio embedded in the incoming stream can be captured. A value of 1
+      lets a sound card reference the audio DAI as <&hdmirx 0>.
+
 required:
   - compatible
   - reg
-- 
2.53.0

[RFC PATCH 2/2] media: synopsys: hdmirx: add HDMI audio capture support

[Igor Paunovic]

The Synopsys DesignWare HDMI RX controller extracts the audio stream
embedded in the incoming HDMI signal and feeds it to an on-SoC I2S
controller. Expose it as an ALSA capture device by registering the
generic hdmi-codec as a child of the controller, so that a
simple-audio-card in the device tree can bind the HDMI RX audio DAI.

The sample rate is recovered from the ACR N/CTS values together with the
measured TMDS character rate. A periodic worker keeps the local audio
reference clock locked to the source by nudging it in small ppm steps to
hold the audio FIFO fill level near its target, which avoids FIFO
under/overflow and the resulting dropped samples.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 244 ++++++++++++++++++
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |   8 +
 2 files changed, 252 insertions(+)

diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
index 9cceffa..f536d17 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
@@ -41,6 +41,8 @@
 #include <media/videobuf2-dma-contig.h>
 #include <media/videobuf2-v4l2.h>
 
+#include <sound/hdmi-codec.h>
+
 #include "snps_hdmirx.h"
 #include "snps_hdmirx_cec.h"
 
@@ -132,6 +134,12 @@ struct snps_hdmirx_dev {
 	struct delayed_work delayed_work_hotplug;
 	struct delayed_work delayed_work_res_change;
 	struct hdmirx_cec *cec;
+	struct platform_device *audio_pdev;
+	struct delayed_work audio_work;
+	u32 audio_clkrate;
+	u32 audio_fs;
+	int audio_pre_state;
+	bool audio_streaming;
 	struct mutex phy_rw_lock; /* to protect phy r/w configuration */
 	struct mutex stream_lock; /* to lock video stream capture */
 	struct mutex work_lock; /* to lock the critical section of hotplug event */
@@ -2650,6 +2658,233 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
 	return 0;
 }
 
+#define HDMIRX_AUDIO_INIT_FIFO_STATE	128
+#define HDMIRX_AUDIO_INIT_STATE		(HDMIRX_AUDIO_INIT_FIFO_STATE * 4)
+
+static const int hdmirx_supported_fs[] = {
+	32000, 44100, 48000, 88200, 96000, 176400, 192000, 768000, -1
+};
+
+static int hdmirx_audio_closest_fs(int fs)
+{
+	int i = 0, fs_t = hdmirx_supported_fs[0];
+
+	while (fs_t > 0) {
+		if (abs(fs - fs_t) <= 2000)
+			return fs_t;
+		fs_t = hdmirx_supported_fs[++i];
+	}
+	return 0;
+}
+
+/* Recover the incoming audio sample rate from the ACR N/CTS + TMDS clock. */
+static u32 hdmirx_audio_fs(struct snps_hdmirx_dev *hdmirx_dev)
+{
+	u64 tmds_clk, fs_audio = 0;
+	u32 acr_cts, acr_n, tmdsqpclk_freq;
+	u32 acr_pb7_4, acr_pb3_0;
+
+	tmdsqpclk_freq = hdmirx_readl(hdmirx_dev, CMU_TMDSQPCLK_FREQ);
+	hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PH2_1);
+	acr_pb7_4 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB3_0);
+	acr_pb3_0 = hdmirx_readl(hdmirx_dev, PKTDEC_ACR_PB7_4);
+	acr_cts = __be32_to_cpu(acr_pb7_4) & 0xfffff;
+	acr_n = (__be32_to_cpu(acr_pb3_0) & 0x0fffff00) >> 8;
+	tmds_clk = tmdsqpclk_freq * 4 * 1000U;
+	if (acr_cts != 0) {
+		fs_audio = div_u64((tmds_clk * acr_n), acr_cts);
+		fs_audio /= 128;
+		fs_audio = hdmirx_audio_closest_fs(fs_audio);
+	}
+	return (u32)fs_audio;
+}
+
+/* Nudge the audio reference clock by +/- ppm to keep the FIFO balanced. */
+static void hdmirx_audio_clk_ppm_inc(struct snps_hdmirx_dev *hdmirx_dev, int ppm)
+{
+	int delta, inc;
+	long rate = hdmirx_dev->audio_clkrate;
+
+	if (ppm < 0) {
+		ppm = -ppm;
+		inc = -1;
+	} else {
+		inc = 1;
+	}
+	delta = (int)div64_u64((u64)rate * ppm + 500000, 1000000);
+	delta *= inc;
+	rate = hdmirx_dev->audio_clkrate + delta;
+	clk_set_rate(hdmirx_dev->clks[1].clk, rate);
+	hdmirx_dev->audio_clkrate = rate;
+}
+
+static int hdmirx_audio_clk_adjust(struct snps_hdmirx_dev *hdmirx_dev,
+				   int total_offset, int single_offset)
+{
+	int schedule_time = 500;
+	int ppm = 10;
+	u32 offset_abs = abs(total_offset);
+
+	if (offset_abs > 200) {
+		ppm += 200;
+		schedule_time -= 100;
+	}
+	if (offset_abs > 100) {
+		ppm += 200;
+		schedule_time -= 100;
+	}
+	if (offset_abs > 32) {
+		ppm += 20;
+		schedule_time -= 100;
+	}
+	if (offset_abs > 16)
+		ppm += 20;
+	if (total_offset > 16 && single_offset > 0)
+		hdmirx_audio_clk_ppm_inc(hdmirx_dev, ppm);
+	else if (total_offset < -16 && single_offset < 0)
+		hdmirx_audio_clk_ppm_inc(hdmirx_dev, -ppm);
+	return schedule_time;
+}
+
+static void hdmirx_audio_fifo_reinit(struct snps_hdmirx_dev *hdmirx_dev)
+{
+	hdmirx_writel(hdmirx_dev, AUDIO_FIFO_CONTROL, 1);
+	usleep_range(200, 210);
+	hdmirx_writel(hdmirx_dev, AUDIO_FIFO_CONTROL, 0);
+}
+
+/*
+ * Periodic worker that locks the local audio clock to the source by keeping
+ * the audio FIFO fill level close to its target, avoiding under/overflow.
+ */
+static void hdmirx_audio_work(struct work_struct *work)
+{
+	struct snps_hdmirx_dev *hdmirx_dev =
+		container_of(to_delayed_work(work), struct snps_hdmirx_dev, audio_work);
+	unsigned long delay = 200;
+	int cur, total, single;
+	u32 fifo, fs;
+
+	fs = hdmirx_audio_fs(hdmirx_dev);
+	fifo = hdmirx_readl(hdmirx_dev, AUDIO_FIFO_STATUS2);
+
+	if (fifo & (AFIFO_UNDERFLOW_ST | AFIFO_OVERFLOW_ST)) {
+		if (fs) {
+			clk_set_rate(hdmirx_dev->clks[1].clk, fs * 128);
+			hdmirx_dev->audio_clkrate = fs * 128;
+			hdmirx_dev->audio_fs = fs;
+		}
+		hdmirx_audio_fifo_reinit(hdmirx_dev);
+		hdmirx_dev->audio_pre_state = 0;
+		goto out;
+	}
+
+	cur = fifo & 0xffff;
+	total = cur - HDMIRX_AUDIO_INIT_STATE;
+	single = cur - hdmirx_dev->audio_pre_state;
+
+	if (fs && abs((int)fs - (int)hdmirx_dev->audio_fs) > 1000) {
+		clk_set_rate(hdmirx_dev->clks[1].clk, fs * 128);
+		hdmirx_dev->audio_clkrate = fs * 128;
+		hdmirx_dev->audio_fs = fs;
+		hdmirx_audio_fifo_reinit(hdmirx_dev);
+		hdmirx_dev->audio_pre_state = 0;
+		goto out;
+	}
+
+	if (cur != 0)
+		delay = hdmirx_audio_clk_adjust(hdmirx_dev, total, single);
+	hdmirx_dev->audio_pre_state = cur;
+out:
+	/* Only re-arm while streaming; avoids a self-reschedule race with
+	 * cancel_delayed_work_sync() in audio_shutdown().
+	 */
+	if (READ_ONCE(hdmirx_dev->audio_streaming))
+		schedule_delayed_work(&hdmirx_dev->audio_work,
+				      msecs_to_jiffies(delay));
+}
+
+static int hdmirx_audio_hw_params(struct device *dev, void *data,
+				  struct hdmi_codec_daifmt *fmt,
+				  struct hdmi_codec_params *hparms)
+{
+	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
+	u32 fs;
+
+	fs = hdmirx_audio_fs(hdmirx_dev);
+	if (!fs)
+		fs = hparms ? hparms->sample_rate : 48000;
+	if (!fs)
+		fs = 48000;
+
+	hdmirx_dev->audio_fs = fs;
+	hdmirx_dev->audio_clkrate = fs * 128;
+	clk_set_rate(hdmirx_dev->clks[1].clk, fs * 128);
+
+	hdmirx_audio_fifo_reinit(hdmirx_dev);
+	hdmirx_writel(hdmirx_dev, AUDIO_FIFO_THR_PASS, HDMIRX_AUDIO_INIT_FIFO_STATE);
+	hdmirx_writel(hdmirx_dev, AUDIO_FIFO_THR,
+		      AFIFO_THR_LOW_QST(0x20) | AFIFO_THR_HIGH_QST(0x160));
+	hdmirx_writel(hdmirx_dev, AUDIO_FIFO_MUTE_THR,
+		      AFIFO_THR_MUTE_LOW_QST(0x8) | AFIFO_THR_MUTE_HIGH_QST(0x178));
+
+	hdmirx_update_bits(hdmirx_dev, AUDIO_PROC_CONFIG0, I2S_EN, I2S_EN);
+	hdmirx_update_bits(hdmirx_dev, GLOBAL_SWENABLE, AUDIO_ENABLE, AUDIO_ENABLE);
+
+	hdmirx_dev->audio_pre_state = 0;
+	WRITE_ONCE(hdmirx_dev->audio_streaming, true);
+	mod_delayed_work(system_unbound_wq, &hdmirx_dev->audio_work,
+			 msecs_to_jiffies(200));
+
+	dev_dbg(dev, "audio hw_params: fs=%u\n", fs);
+	return 0;
+}
+
+static void hdmirx_audio_shutdown(struct device *dev, void *data)
+{
+	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
+
+	WRITE_ONCE(hdmirx_dev->audio_streaming, false);
+	cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+	hdmirx_update_bits(hdmirx_dev, GLOBAL_SWENABLE, AUDIO_ENABLE, 0);
+}
+
+static int hdmirx_audio_get_dai_id(struct snd_soc_component *component,
+				   struct device_node *endpoint,
+				   void *data)
+{
+	return 0;
+}
+
+static const struct hdmi_codec_ops hdmirx_audio_codec_ops = {
+	.hw_params = hdmirx_audio_hw_params,
+	.audio_shutdown = hdmirx_audio_shutdown,
+	.get_dai_id = hdmirx_audio_get_dai_id,
+};
+
+static int hdmirx_register_audio_device(struct snps_hdmirx_dev *hdmirx_dev)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &hdmirx_audio_codec_ops,
+		.i2s = 1,
+		.no_i2s_playback = 1,
+		.max_i2s_channels = 8,
+		.data = hdmirx_dev,
+	};
+	struct platform_device_info pdevinfo = {
+		.parent = hdmirx_dev->dev,
+		.id = PLATFORM_DEVID_AUTO,
+		.name = HDMI_CODEC_DRV_NAME,
+		.data = &codec_data,
+		.size_data = sizeof(codec_data),
+		.dma_mask = DMA_BIT_MASK(32),
+	};
+
+	hdmirx_dev->audio_pdev = platform_device_register_full(&pdevinfo);
+
+	return PTR_ERR_OR_ZERO(hdmirx_dev->audio_pdev);
+}
+
 static int hdmirx_probe(struct platform_device *pdev)
 {
 	struct snps_hdmirx_dev *hdmirx_dev;
@@ -2701,6 +2936,7 @@ static int hdmirx_probe(struct platform_device *pdev)
 			  hdmirx_delayed_work_hotplug);
 	INIT_DELAYED_WORK(&hdmirx_dev->delayed_work_res_change,
 			  hdmirx_delayed_work_res_change);
+	INIT_DELAYED_WORK(&hdmirx_dev->audio_work, hdmirx_audio_work);
 
 	hdmirx_dev->cur_fmt_fourcc = V4L2_PIX_FMT_BGR24;
 	hdmirx_dev->timings = cea640x480;
@@ -2769,6 +3005,10 @@ static int hdmirx_probe(struct platform_device *pdev)
 						       V4L2_DEBUGFS_IF_AVI, hdmirx_dev,
 						       hdmirx_debugfs_if_read);
 
+	ret = hdmirx_register_audio_device(hdmirx_dev);
+	if (ret)
+		dev_warn(dev, "failed to register HDMI audio codec: %d\n", ret);
+
 	return 0;
 
 err_unreg_video_dev:
@@ -2788,6 +3028,10 @@ static void hdmirx_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
 
+	cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+	if (hdmirx_dev->audio_pdev)
+		platform_device_unregister(hdmirx_dev->audio_pdev);
+
 	v4l2_debugfs_if_free(hdmirx_dev->infoframes);
 	debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
 
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
index 31b887e..a99f54f 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
@@ -81,6 +81,7 @@
 #define DATAPATH_ENABLE				BIT(12)
 #define PKTFIFO_ENABLE				BIT(11)
 #define AVPUNIT_ENABLE				BIT(8)
+#define AUDIO_ENABLE				BIT(9)
 #define MAIN_ENABLE				BIT(0)
 #define GLOBAL_TIMER_REF_BASE			0x0028
 #define CORE_CONFIG				0x0050
@@ -177,20 +178,27 @@
 #define VPROC_FMT_OVR_VALUE(x)			UPDATE(x, 6, 4)
 #define VPROC_FMT_OVR_EN			BIT(0)
 
+#define AUDIO_FIFO_CONFIG			0x0460
 #define AFIFO_FILL_RESTART			BIT(0)
+#define AUDIO_FIFO_CONTROL			0x0464
 #define AFIFO_INIT_P				BIT(0)
+#define AUDIO_FIFO_THR_PASS			0x0468
+#define AUDIO_FIFO_THR				0x046c
 #define AFIFO_THR_LOW_QST_MASK			GENMASK(25, 16)
 #define AFIFO_THR_LOW_QST(x)			UPDATE(x, 25, 16)
 #define AFIFO_THR_HIGH_QST_MASK			GENMASK(9, 0)
 #define AFIFO_THR_HIGH_QST(x)			UPDATE(x, 9, 0)
+#define AUDIO_FIFO_MUTE_THR			0x0470
 #define AFIFO_THR_MUTE_LOW_QST_MASK		GENMASK(25, 16)
 #define AFIFO_THR_MUTE_LOW_QST(x)		UPDATE(x, 25, 16)
 #define AFIFO_THR_MUTE_HIGH_QST_MASK		GENMASK(9, 0)
 #define AFIFO_THR_MUTE_HIGH_QST(x)		UPDATE(x, 9, 0)
 
+#define AUDIO_FIFO_STATUS2			0x0478
 #define AFIFO_UNDERFLOW_ST			BIT(25)
 #define AFIFO_OVERFLOW_ST			BIT(24)
 
+#define AUDIO_PROC_CONFIG0			0x0480
 #define SPEAKER_ALLOC_OVR_EN			BIT(16)
 #define I2S_BPCUV_EN				BIT(4)
 #define SPDIF_EN				BIT(2)
-- 
2.53.0

Re: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Royal Net]

> Should the constraint be set to 0 to follow standard device tree
> norms?

Fair point. The current RFC wires up a single I2S DAI, and for a
single-DAI provider 0 is indeed the norm -- rockchip,dw-hdmi does
exactly that for its one DAI on the TX side.

The reason I picked 1 is that the controller also has an S/PDIF
audio output path (the vendor driver exposes both I2S and S/PDIF
DAIs through hdmi-codec), so a DAI index becomes meaningful if that
second path is wired up later.

Unless the DT maintainers prefer keeping 1 from the start to avoid
changing the cell count if/when the S/PDIF DAI is added, I'll switch
to const: 0 in the next revision -- please let me know which you'd
prefer.

Igor

On Wed, Jul 15, 2026 at 7:54 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Incorrect cell count constraint for single-DAI provider.
> --
>
> --- Patch [1]: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells ---
> commit e6b0bac0b97f568ce749084e6c1e728bcd258b10
> Author: Igor Paunovic <royalnet026@gmail.com>
> dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
>
> This commit adds the #sound-dai-cells property to the HDMI RX controller
> bindings, enabling a generic audio codec to capture incoming embedded audio.
>
> > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > index b7f6c87d0e06a..83c10d9997c8e 100644
> > --- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > @@ -78,6 +78,13 @@ properties:
> >        The phandle of the syscon node for the Video Output GRF register
> >        to enable EDID transfer through SDAIN and SCLIN.
> >
> > +  "#sound-dai-cells":
> > +    const: 1
> > +    description:
> > +      The HDMI RX controller registers a generic HDMI audio codec so the
> > +      audio embedded in the incoming stream can be captured. A value of 1
> > +      lets a sound card reference the audio DAI as <&hdmirx 0>.
>
> [Severity: Medium]
> Is this cell count constraint correct for a single-DAI provider?
>
> Looking at properties:#sound-dai-cells, the hardware exposes only a single
> generic audio DAI. Requiring a cell count of 1 forces device tree nodes to
> use a non-standard syntax like <&hdmirx 0> instead of just <&hdmirx> for a
> single resource.
>
> Should the constraint be set to 0 to follow standard device tree norms?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=1

Re: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Sebastian Reichel]

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

Hello Igor,

On Wed, Jul 15, 2026 at 08:55:37AM +0200, Royal Net wrote:
> > Should the constraint be set to 0 to follow standard device tree
> > norms?
> 
> Fair point. The current RFC wires up a single I2S DAI, and for a
> single-DAI provider 0 is indeed the norm -- rockchip,dw-hdmi does
> exactly that for its one DAI on the TX side.
> 
> The reason I picked 1 is that the controller also has an S/PDIF
> audio output path (the vendor driver exposes both I2S and S/PDIF
> DAIs through hdmi-codec), so a DAI index becomes meaningful if that
> second path is wired up later.
> 
> Unless the DT maintainers prefer keeping 1 from the start to avoid
> changing the cell count if/when the S/PDIF DAI is added, I'll switch
> to const: 0 in the next revision -- please let me know which you'd
> prefer.

It should be 1 from the start as the DT binding is supposed to
describe the hardware and be stable. But in the next version you
should look into updating the commit descriptions to be less about
the kernel driver and more about the hardware. Maybe something like

> The HDMI RX controller has two digital audio interfaces, one for
> I2S and one for S/PDIF. Allow referencing connections from audio
> codecs to these interfaces by documenting the missing #sound-dai-cells
> property.

Apart from that it makes sense to document that argument 0 means
I2S and argument 1 means S/PDIF interface in the property description.

Last but not least drop "A value of 1 lets a sound card reference
the audio DAI as <&hdmirx 0>.". Instead you can add this to the
example section at the end of the file.

Greetings,

-- Sebastian

> 
> Igor
> 
> On Wed, Jul 15, 2026 at 7:54 AM <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Incorrect cell count constraint for single-DAI provider.
> > --
> >
> > --- Patch [1]: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells ---
> > commit e6b0bac0b97f568ce749084e6c1e728bcd258b10
> > Author: Igor Paunovic <royalnet026@gmail.com>
> > dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
> >
> > This commit adds the #sound-dai-cells property to the HDMI RX controller
> > bindings, enabling a generic audio codec to capture incoming embedded audio.
> >
> > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > index b7f6c87d0e06a..83c10d9997c8e 100644
> > > --- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > @@ -78,6 +78,13 @@ properties:
> > >        The phandle of the syscon node for the Video Output GRF register
> > >        to enable EDID transfer through SDAIN and SCLIN.
> > >
> > > +  "#sound-dai-cells":
> > > +    const: 1
> > > +    description:
> > > +      The HDMI RX controller registers a generic HDMI audio codec so the
> > > +      audio embedded in the incoming stream can be captured. A value of 1
> > > +      lets a sound card reference the audio DAI as <&hdmirx 0>.
> >
> > [Severity: Medium]
> > Is this cell count constraint correct for a single-DAI provider?
> >
> > Looking at properties:#sound-dai-cells, the hardware exposes only a single
> > generic audio DAI. Requiring a cell count of 1 forces device tree nodes to
> > use a non-standard syntax like <&hdmirx 0> instead of just <&hdmirx> for a
> > single resource.
> >
> > Should the constraint be set to 0 to follow standard device tree norms?
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=1

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

Re: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Igor Paunovic]

Hello Sebastian,

> It should be 1 from the start as the DT binding is supposed to
> describe the hardware and be stable.

Thanks -- keeping const: 1 then.

> But in the next version you should look into updating the commit
> descriptions to be less about the kernel driver and more about
> the hardware.

Will do. I'll use your wording as the base for the v2 commit
message, document that argument 0 is the I2S and 1 the S/PDIF
interface in the property description, and move the consumer
reference (sound-dai = <&hdmi_receiver 0>) into the example
section.

For completeness: the driver part of the series currently wires
up only the I2S interface (DAI 0); the binding now describes both
interfaces as per the hardware.

Thanks for the review!

Igor

On Wed, Jul 15, 2026 at 3:00 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hello Igor,
>
> On Wed, Jul 15, 2026 at 08:55:37AM +0200, Royal Net wrote:
> > > Should the constraint be set to 0 to follow standard device tree
> > > norms?
> >
> > Fair point. The current RFC wires up a single I2S DAI, and for a
> > single-DAI provider 0 is indeed the norm -- rockchip,dw-hdmi does
> > exactly that for its one DAI on the TX side.
> >
> > The reason I picked 1 is that the controller also has an S/PDIF
> > audio output path (the vendor driver exposes both I2S and S/PDIF
> > DAIs through hdmi-codec), so a DAI index becomes meaningful if that
> > second path is wired up later.
> >
> > Unless the DT maintainers prefer keeping 1 from the start to avoid
> > changing the cell count if/when the S/PDIF DAI is added, I'll switch
> > to const: 0 in the next revision -- please let me know which you'd
> > prefer.
>
> It should be 1 from the start as the DT binding is supposed to
> describe the hardware and be stable. But in the next version you
> should look into updating the commit descriptions to be less about
> the kernel driver and more about the hardware. Maybe something like
>
> > The HDMI RX controller has two digital audio interfaces, one for
> > I2S and one for S/PDIF. Allow referencing connections from audio
> > codecs to these interfaces by documenting the missing #sound-dai-cells
> > property.
>
> Apart from that it makes sense to document that argument 0 means
> I2S and argument 1 means S/PDIF interface in the property description.
>
> Last but not least drop "A value of 1 lets a sound card reference
> the audio DAI as <&hdmirx 0>.". Instead you can add this to the
> example section at the end of the file.
>
> Greetings,
>
> -- Sebastian
>
> >
> > Igor
> >
> > On Wed, Jul 15, 2026 at 7:54 AM <sashiko-bot@kernel.org> wrote:
> > >
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [Medium] Incorrect cell count constraint for single-DAI provider.
> > > --
> > >
> > > --- Patch [1]: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells ---
> > > commit e6b0bac0b97f568ce749084e6c1e728bcd258b10
> > > Author: Igor Paunovic <royalnet026@gmail.com>
> > > dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
> > >
> > > This commit adds the #sound-dai-cells property to the HDMI RX controller
> > > bindings, enabling a generic audio codec to capture incoming embedded audio.
> > >
> > > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > index b7f6c87d0e06a..83c10d9997c8e 100644
> > > > --- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > @@ -78,6 +78,13 @@ properties:
> > > >        The phandle of the syscon node for the Video Output GRF register
> > > >        to enable EDID transfer through SDAIN and SCLIN.
> > > >
> > > > +  "#sound-dai-cells":
> > > > +    const: 1
> > > > +    description:
> > > > +      The HDMI RX controller registers a generic HDMI audio codec so the
> > > > +      audio embedded in the incoming stream can be captured. A value of 1
> > > > +      lets a sound card reference the audio DAI as <&hdmirx 0>.
> > >
> > > [Severity: Medium]
> > > Is this cell count constraint correct for a single-DAI provider?
> > >
> > > Looking at properties:#sound-dai-cells, the hardware exposes only a single
> > > generic audio DAI. Requiring a cell count of 1 forces device tree nodes to
> > > use a non-standard syntax like <&hdmirx 0> instead of just <&hdmirx> for a
> > > single resource.
> > >
> > > Should the constraint be set to 0 to follow standard device tree norms?
> > >
> > > --
> > > Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=1

Re: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells

[Sebastian Reichel]

Hi,

On Wed, Jul 15, 2026 at 03:19:54PM +0200, Igor Paunovic wrote:
> For completeness: the driver part of the series currently wires
> up only the I2S interface (DAI 0); the binding now describes both
> interfaces as per the hardware.

Not implementing all features in the driver is perfectly fine.
Ideally you return -ENOTSUP if somebody tries to use the S/PDIF
DAI interface instead of I2S.

Greetings,

-- Sebastian

> 
> Thanks for the review!
> 
> Igor
> 
> On Wed, Jul 15, 2026 at 3:00 PM Sebastian Reichel
> <sebastian.reichel@collabora.com> wrote:
> >
> > Hello Igor,
> >
> > On Wed, Jul 15, 2026 at 08:55:37AM +0200, Royal Net wrote:
> > > > Should the constraint be set to 0 to follow standard device tree
> > > > norms?
> > >
> > > Fair point. The current RFC wires up a single I2S DAI, and for a
> > > single-DAI provider 0 is indeed the norm -- rockchip,dw-hdmi does
> > > exactly that for its one DAI on the TX side.
> > >
> > > The reason I picked 1 is that the controller also has an S/PDIF
> > > audio output path (the vendor driver exposes both I2S and S/PDIF
> > > DAIs through hdmi-codec), so a DAI index becomes meaningful if that
> > > second path is wired up later.
> > >
> > > Unless the DT maintainers prefer keeping 1 from the start to avoid
> > > changing the cell count if/when the S/PDIF DAI is added, I'll switch
> > > to const: 0 in the next revision -- please let me know which you'd
> > > prefer.
> >
> > It should be 1 from the start as the DT binding is supposed to
> > describe the hardware and be stable. But in the next version you
> > should look into updating the commit descriptions to be less about
> > the kernel driver and more about the hardware. Maybe something like
> >
> > > The HDMI RX controller has two digital audio interfaces, one for
> > > I2S and one for S/PDIF. Allow referencing connections from audio
> > > codecs to these interfaces by documenting the missing #sound-dai-cells
> > > property.
> >
> > Apart from that it makes sense to document that argument 0 means
> > I2S and argument 1 means S/PDIF interface in the property description.
> >
> > Last but not least drop "A value of 1 lets a sound card reference
> > the audio DAI as <&hdmirx 0>.". Instead you can add this to the
> > example section at the end of the file.
> >
> > Greetings,
> >
> > -- Sebastian
> >
> > >
> > > Igor
> > >
> > > On Wed, Jul 15, 2026 at 7:54 AM <sashiko-bot@kernel.org> wrote:
> > > >
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [Medium] Incorrect cell count constraint for single-DAI provider.
> > > > --
> > > >
> > > > --- Patch [1]: [RFC PATCH 1/2] dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells ---
> > > > commit e6b0bac0b97f568ce749084e6c1e728bcd258b10
> > > > Author: Igor Paunovic <royalnet026@gmail.com>
> > > > dt-bindings: media: snps,dw-hdmi-rx: add #sound-dai-cells
> > > >
> > > > This commit adds the #sound-dai-cells property to the HDMI RX controller
> > > > bindings, enabling a generic audio codec to capture incoming embedded audio.
> > > >
> > > > > diff --git a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > index b7f6c87d0e06a..83c10d9997c8e 100644
> > > > > --- a/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/snps,dw-hdmi-rx.yaml
> > > > > @@ -78,6 +78,13 @@ properties:
> > > > >        The phandle of the syscon node for the Video Output GRF register
> > > > >        to enable EDID transfer through SDAIN and SCLIN.
> > > > >
> > > > > +  "#sound-dai-cells":
> > > > > +    const: 1
> > > > > +    description:
> > > > > +      The HDMI RX controller registers a generic HDMI audio codec so the
> > > > > +      audio embedded in the incoming stream can be captured. A value of 1
> > > > > +      lets a sound card reference the audio DAI as <&hdmirx 0>.
> > > >
> > > > [Severity: Medium]
> > > > Is this cell count constraint correct for a single-DAI provider?
> > > >
> > > > Looking at properties:#sound-dai-cells, the hardware exposes only a single
> > > > generic audio DAI. Requiring a cell count of 1 forces device tree nodes to
> > > > use a non-standard syntax like <&hdmirx 0> instead of just <&hdmirx> for a
> > > > single resource.
> > > >
> > > > Should the constraint be set to 0 to follow standard device tree norms?
> > > >
> > > > --
> > > > Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=1