[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 2/2] media: synopsys: hdmirx: add HDMI audio capture support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Passing an ERR_PTR to platform_device_unregister() causes a kernel panic during module removal.
- [High] Flawed teardown ordering and failure to clear the audio_streaming flag in hdmirx_remove() allows the delayed work to re-arm, leading to a Use-After-Free.
- [High] Concurrent unprotected access to shared audio state variables between ALSA `hw_params` and the background worker.
- [Medium] Unconditional access to `clks[1]` causes an out-of-bounds read if the Device Tree specifies only one clock.
- [Low] The audio worker item silently migrates from an unbound workqueue to the default CPU-bound workqueue after its first execution.
--

--- Patch [2]: [RFC PATCH 2/2] media: synopsys: hdmirx: add HDMI audio capture support ---
commit a0cbe815be5692f1b4e770ef5f613ff1fa0b7862
Author: Igor Paunovic <royalnet026@gmail.com>

media: synopsys: hdmirx: add HDMI audio capture support

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.

> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c

[ ... ]

> +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);

[Severity: Medium]
Does this code access the clock array out of bounds if the device tree only
provides one clock?

The probe function uses devm_clk_bulk_get_all() which only guarantees
num_clks >= 1, but this unconditionally accesses index 1.

> +	hdmirx_dev->audio_clkrate = rate;
> +}

[ ... ]

> +static void hdmirx_audio_work(struct work_struct *work)
> +{

[ ... ]

> +	/* 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));

[Severity: Low]
Does this cause the worker to silently migrate to the default system_wq?

The worker is initially queued on system_unbound_wq using mod_delayed_work(),
but schedule_delayed_work() implicitly targets the default CPU-bound
workqueue.

> +}
> +
> +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;

[Severity: High]
Can concurrent ALSA hw_params ioctl calls race with the background worker here?

ALSA allows consecutive hw_params calls, which could modify audio_fs,
audio_clkrate, and audio_pre_state locklessly while the worker is already
executing on another CPU. 

Could this concurrent access result in an invalid clock frequency calculation?

> +	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;
> +}

[ ... ]

> @@ -2765,6 +3001,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);
> +

[Severity: High]
If platform_device_register_full() fails in hdmirx_register_audio_device(), 
it returns an ERR_PTR which is stored in hdmirx_dev->audio_pdev. 

Since this failure only logs a warning and allows the probe to succeed, won't 
the check in hdmirx_remove() evaluate to true?

if (hdmirx_dev->audio_pdev)
    platform_device_unregister(hdmirx_dev->audio_pdev);
            
Will this cause a kernel panic when platform_device_unregister() attempts to 
dereference the ERR_PTR?

>  	return 0;
>  
>  err_unreg_video_dev:
> @@ -2784,6 +3024,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);
> +

[Severity: High]
Could the worker re-arm itself after this cancelation?

If the worker is running during removal, cancel_delayed_work_sync() waits for
it to finish. However, because audio_streaming is not set to false prior to
this call, the worker will see audio_streaming == true and re-arm itself.

Additionally, since platform_device_unregister() is called after the timer
cancelation, could userspace trigger a concurrent hw_params ioctl and re-arm
the timer through that path?

Will the re-armed timer execute on freed memory after the device is removed?

>  	v4l2_debugfs_if_free(hdmirx_dev->infoframes);
>  	debugfs_remove_recursive(hdmirx_dev->debugfs_dir);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=2

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

[sashiko-bot]

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 2/2] media: synopsys: hdmirx: add HDMI audio capture support

[Royal Net]

Thanks for the thorough pass -- all five are in the new code, so all
five are mine to fix. Ack on each:

> - [High] Passing an ERR_PTR to platform_device_unregister() causes
> a kernel panic during module removal.

Confirmed. hdmirx_register_audio_device() leaves the ERR_PTR in
audio_pdev when registration fails (probe intentionally only warns),
and the NULL check in hdmirx_remove() doesn't catch an ERR_PTR. Will
store NULL on failure so the existing check works.

> - [High] Flawed teardown ordering and failure to clear the
> audio_streaming flag in hdmirx_remove() allows the delayed work
> to re-arm, leading to a Use-After-Free.

Confirmed. The self-rearm guard was added for audio_shutdown() but
remove() never clears audio_streaming, so a worker instance running
at removal time can re-arm after cancel_delayed_work_sync() returns.
Will clear the flag first, unregister the codec device (tearing down
the stream), and cancel the work last.

> - [High] Concurrent unprotected access to shared audio state
> variables between ALSA `hw_params` and the background worker.

Confirmed. A repeated hw_params without an intervening shutdown can
race the worker over audio_fs/audio_clkrate/audio_pre_state. Plan is
to stop the worker (clear the flag + cancel_delayed_work_sync) at the
top of hw_params before touching shared state, which keeps the worker
the only writer while it is armed and avoids introducing a lock.
> - [Medium] Unconditional access to `clks[1]` causes an
> out-of-bounds read if the Device Tree specifies only one clock.

Agreed -- index-based access is fragile even though the binding lists
the audio clock. Will resolve the "audio" clock by name from the bulk
clock data once at probe time and cache the pointer.
> - [Low] The audio worker item silently migrates from an unbound
> workqueue to the default CPU-bound workqueue after its first
> execution.

Confirmed; the re-arm path uses schedule_delayed_work() while the
initial queueing uses system_unbound_wq. Will use
queue_delayed_work(system_unbound_wq, ...) consistently.

All five will be addressed in the next revision, together with any
design-level feedback on the RFC itself.

Igor

On Wed, Jul 15, 2026 at 7:48 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Passing an ERR_PTR to platform_device_unregister() causes a kernel panic during module removal.
> - [High] Flawed teardown ordering and failure to clear the audio_streaming flag in hdmirx_remove() allows the delayed work to re-arm, leading to a Use-After-Free.
> - [High] Concurrent unprotected access to shared audio state variables between ALSA `hw_params` and the background worker.
> - [Medium] Unconditional access to `clks[1]` causes an out-of-bounds read if the Device Tree specifies only one clock.
> - [Low] The audio worker item silently migrates from an unbound workqueue to the default CPU-bound workqueue after its first execution.
> --
>
> --- Patch [2]: [RFC PATCH 2/2] media: synopsys: hdmirx: add HDMI audio capture support ---
> commit a0cbe815be5692f1b4e770ef5f613ff1fa0b7862
> Author: Igor Paunovic <royalnet026@gmail.com>
>
> media: synopsys: hdmirx: add HDMI audio capture support
>
> 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.
>
> > diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
>
> [ ... ]
>
> > +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);
>
> [Severity: Medium]
> Does this code access the clock array out of bounds if the device tree only
> provides one clock?
>
> The probe function uses devm_clk_bulk_get_all() which only guarantees
> num_clks >= 1, but this unconditionally accesses index 1.
>
> > +     hdmirx_dev->audio_clkrate = rate;
> > +}
>
> [ ... ]
>
> > +static void hdmirx_audio_work(struct work_struct *work)
> > +{
>
> [ ... ]
>
> > +     /* 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));
>
> [Severity: Low]
> Does this cause the worker to silently migrate to the default system_wq?
>
> The worker is initially queued on system_unbound_wq using mod_delayed_work(),
> but schedule_delayed_work() implicitly targets the default CPU-bound
> workqueue.
>
> > +}
> > +
> > +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;
>
> [Severity: High]
> Can concurrent ALSA hw_params ioctl calls race with the background worker here?
>
> ALSA allows consecutive hw_params calls, which could modify audio_fs,
> audio_clkrate, and audio_pre_state locklessly while the worker is already
> executing on another CPU.
>
> Could this concurrent access result in an invalid clock frequency calculation?
>
> > +     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;
> > +}
>
> [ ... ]
>
> > @@ -2765,6 +3001,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);
> > +
>
> [Severity: High]
> If platform_device_register_full() fails in hdmirx_register_audio_device(),
> it returns an ERR_PTR which is stored in hdmirx_dev->audio_pdev.
>
> Since this failure only logs a warning and allows the probe to succeed, won't
> the check in hdmirx_remove() evaluate to true?
>
> if (hdmirx_dev->audio_pdev)
>     platform_device_unregister(hdmirx_dev->audio_pdev);
>
> Will this cause a kernel panic when platform_device_unregister() attempts to
> dereference the ERR_PTR?
>
> >       return 0;
> >
> >  err_unreg_video_dev:
> > @@ -2784,6 +3024,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);
> > +
>
> [Severity: High]
> Could the worker re-arm itself after this cancelation?
>
> If the worker is running during removal, cancel_delayed_work_sync() waits for
> it to finish. However, because audio_streaming is not set to false prior to
> this call, the worker will see audio_streaming == true and re-arm itself.
>
> Additionally, since platform_device_unregister() is called after the timer
> cancelation, could userspace trigger a concurrent hw_params ioctl and re-arm
> the timer through that path?
>
> Will the re-armed timer execute on freed memory after the device is removed?
>
> >       v4l2_debugfs_if_free(hdmirx_dev->infoframes);
> >       debugfs_remove_recursive(hdmirx_dev->debugfs_dir);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260715051939.64652-1-royalnet026@gmail.com?part=2

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