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

[PATCH v2 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 extracts
the audio embedded in the incoming HDMI stream and feeds it to an
on-SoC I2S interface; patch 1 documents #sound-dai-cells in the
binding and patch 2 registers the generic hdmi-codec as a child of the
controller, recovers the audio sample rate from ACR N/CTS and the
measured TMDS character rate, and keeps the local audio reference
clock locked to the source with a periodic FIFO-level worker.

v2 addresses all review feedback on the RFC: Sebastian Reichel's
binding comments (hardware-focused description, documented DAI
indexes, consumer reference in the example) and the S/PDIF DAI
handling, Dmitry Osipenko's platform-device registration comment, the
Sashiko AI review findings on the driver's error and teardown paths,
and the Media CI sparse warnings. Per-patch changelogs are below each
patch.

Re-validated on an Orange Pi 5 Plus (RK3588, i2s7_8ch as the capture
DAI): stable capture at 44.1 kHz and 48 kHz, sample rate switching,
repeated capture start/stop cycles, and abrupt client termination
without worker leaks.

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       |   9 +
 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 280 ++++++++++++++++++
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |   8 +
 3 files changed, 297 insertions(+)

-- 
2.53.0

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

[Igor Paunovic]

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.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Changes in v2 (all suggested by Sebastian Reichel):
- reworded the commit message to describe the hardware
- property description now documents the DAI indexes (0 = I2S,
  1 = S/PDIF)
- moved the consumer reference into the binding example

 .../devicetree/bindings/media/snps,dw-hdmi-rx.yaml       | 9 +++++++++
 1 file changed, 9 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..fa6cd0d 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 has two digital audio interfaces, one for
+      I2S and one for S/PDIF. The DAI cell selects the interface, 0 for
+      I2S and 1 for S/PDIF.
+
 required:
   - compatible
   - reg
@@ -129,4 +136,6 @@ examples:
       pinctrl-0 = <&hdmim1_rx_cec &hdmim1_rx_hpdin &hdmim1_rx_scl &hdmim1_rx_sda &hdmirx_5v_detection>;
       pinctrl-names = "default";
       hpd-gpios = <&gpio1 22 GPIO_ACTIVE_LOW>;
+      /* referenced by a sound card as <&hdmi_receiver 0> (0: I2S, 1: S/PDIF) */
+      #sound-dai-cells = <1>;
     };
-- 
2.53.0

[PATCH v2 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.

The controller's S/PDIF interface is registered as a second DAI so the
DAI indexes match the binding (0 = I2S, 1 = S/PDIF), and is rejected
with -EOPNOTSUPP in hw_params() until that path is wired up.

Signed-off-by: Igor Paunovic <royalnet026@gmail.com>
---
Changes in v2:
- register the S/PDIF DAI so the indexes match the binding and reject
  it with -EOPNOTSUPP until wired up (Sebastian Reichel)
- use platform_device_register_data() and drop the fixed 32-bit DMA
  mask (Dmitry Osipenko)
- don't leave an ERR_PTR in audio_pdev on registration failure
- fix teardown ordering in remove(): clear the streaming flag first,
  unregister the codec, cancel the worker last
- stop the worker before reprogramming shared state in hw_params()
- look up the "audio" clock by name instead of indexing clks[1]
- keep the worker on system_unbound_wq when re-arming
- fix sparse warnings in the ACR register reads (Media CI)

 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 280 ++++++++++++++++++
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |   8 +
 2 files changed, 288 insertions(+)

diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
index 9cceffa..78f88af 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,13 @@ 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 clk *audio_clk;
+	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 */
@@ -2283,6 +2292,13 @@ static int hdmirx_parse_dt(struct snps_hdmirx_dev *hdmirx_dev)
 	if (hdmirx_dev->num_clks < 1)
 		return -ENODEV;
 
+	for (int i = 0; i < hdmirx_dev->num_clks; i++) {
+		if (!strcmp(hdmirx_dev->clks[i].id, "audio")) {
+			hdmirx_dev->audio_clk = hdmirx_dev->clks[i].clk;
+			break;
+		}
+	}
+
 	hdmirx_dev->resets[HDMIRX_RST_A].id = "axi";
 	hdmirx_dev->resets[HDMIRX_RST_P].id = "apb";
 	hdmirx_dev->resets[HDMIRX_RST_REF].id = "ref";
@@ -2650,6 +2666,255 @@ 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((__force __be32)acr_pb7_4) & 0xfffff;
+	acr_n = (be32_to_cpu((__force __be32)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->audio_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->audio_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->audio_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
+	 * the cancel_delayed_work_sync() callers (hw_params, audio_shutdown
+	 * and remove).
+	 */
+	if (READ_ONCE(hdmirx_dev->audio_streaming))
+		queue_delayed_work(system_unbound_wq, &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;
+
+	/* Only the I2S interface (DAI 0) is wired up so far. */
+	if (fmt->fmt == HDMI_SPDIF)
+		return -EOPNOTSUPP;
+
+	/*
+	 * Stop the worker before touching the shared audio state; it is
+	 * re-armed below once the new parameters are in place.
+	 */
+	WRITE_ONCE(hdmirx_dev->audio_streaming, false);
+	cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+
+	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->audio_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,
+		/*
+		 * The controller also has an S/PDIF audio interface (DAI 1 in
+		 * the binding). Register it so DAI indexes match the binding,
+		 * but reject its use in hw_params() until it is wired up.
+		 */
+		.spdif = 1,
+		.no_spdif_playback = 1,
+		.data = hdmirx_dev,
+	};
+	struct platform_device *audio_pdev;
+
+	if (!hdmirx_dev->audio_clk)
+		return -ENODEV;
+
+	audio_pdev = platform_device_register_data(hdmirx_dev->dev,
+						   HDMI_CODEC_DRV_NAME,
+						   PLATFORM_DEVID_AUTO,
+						   &codec_data, sizeof(codec_data));
+	if (IS_ERR(audio_pdev))
+		return PTR_ERR(audio_pdev);
+
+	hdmirx_dev->audio_pdev = audio_pdev;
+
+	return 0;
+}
+
 static int hdmirx_probe(struct platform_device *pdev)
 {
 	struct snps_hdmirx_dev *hdmirx_dev;
@@ -2701,6 +2966,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 +3035,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 +3058,16 @@ static void hdmirx_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);
 
+	/*
+	 * Clear the streaming flag first so a running worker cannot re-arm
+	 * itself, then tear down the codec (which stops any active stream)
+	 * before the final cancel.
+	 */
+	WRITE_ONCE(hdmirx_dev->audio_streaming, false);
+	if (hdmirx_dev->audio_pdev)
+		platform_device_unregister(hdmirx_dev->audio_pdev);
+	cancel_delayed_work_sync(&hdmirx_dev->audio_work);
+
 	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: [PATCH v2 2/2] media: synopsys: hdmirx: add HDMI audio capture support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Severe logical and mathematical errors in Audio Clock Regeneration (ACR) N and CTS parsing, leading to hardware clock recovery failure.
- [High] Dummy `hdmirx_audio_get_dai_id` callback unconditionally returns 0, breaking S/PDIF DAI OF graph mapping.
--

--- Patch [2]: [PATCH v2 2/2] media: synopsys: hdmirx: add HDMI audio capture support ---
commit c4b60dd6e2a0b110adfab2b5a2ccf3d214ed0d05
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, so that a
simple-audio-card in the device tree can bind the HDMI RX audio DAI.

> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index 25f8ca0d6d946..acabd43664c10 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c

[ ... ]

> @@ -2646,6 +2662,255 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,

[ ... ]

> +/* 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((__force __be32)acr_pb7_4) & 0xfffff;
> +	acr_n = (be32_to_cpu((__force __be32)acr_pb3_0) & 0x0fffff00) >> 8;

[Severity: High]
Does this code incorrectly parse the ACR N and CTS values?

The variables acr_pb7_4 and acr_pb3_0 are assigned the swapped register
contents (PKTDEC_ACR_PB3_0 is assigned to acr_pb7_4, and vice versa).

Additionally, applying be32_to_cpu() to the native-endian result of
hdmirx_readl() will cause big-endian byte-swapping on little-endian
architectures. Because of the swapped assignment and byte reversal, the
bitwise shift operations extract completely incorrect bytes.

Will this cause hardware clock regeneration to fail and silently fall back
to the ALSA default sample rate (48kHz)?

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

[ ... ]

> +static int hdmirx_audio_get_dai_id(struct snd_soc_component *component,
> +				   struct device_node *endpoint,
> +				   void *data)
> +{
> +	return 0;
> +}

[Severity: High]
Does this dummy callback break the S/PDIF DAI OF graph mapping?

The driver registers two audio DAIs (0 for I2S, 1 for S/PDIF) to match the
DT binding. However, this callback unconditionally returns 0, which overrides
the standard DAI index parsing in the ASoC core.

Consequently, any DT endpoint requesting S/PDIF (reg = <1>) will be
incorrectly forced to map to DAI 0 (I2S). This bypasses the intended
-EOPNOTSUPP rejection in hdmirx_audio_hw_params() and erroneously configures
the hardware for I2S when S/PDIF was requested.

Could this dummy callback simply be removed to allow the core to map the reg
property to the correct DAI ID naturally?

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

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

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

[Igor Paunovic]

> Does this code incorrectly parse the ACR N and CTS values?

No -- but the variable naming makes it look like it does, so this is
worth cleaning up. The packet decoder stores the ACR subpacket bytes
with packet byte 0 in register bits [7:0], so the byte swap is what
lines the bytes up for extraction: CTS is packet bytes 1-3 (register
PKTDEC_ACR_PB3_0) and N is packet bytes 4-6 (PKTDEC_ACR_PB7_4), and
the masks then take exactly the 20-bit fields. The two locals are
named after the wrong registers (inherited from the vendor driver),
which is confusing but has no functional effect.

The path is also validated on hardware: with a 44.1 kHz source the
card reports 44.1 kHz rather than falling back to the 48 kHz default,
and sample rate switches are followed -- neither would work if the
parsing were broken. I'll rename the locals and add a comment about
the byte packing in the next revision.

> Could this dummy callback simply be removed to allow the core to
> map the reg property to the correct DAI ID naturally?

The #sound-dai-cells path is not affected -- there the DAI index
comes from the phandle cell, so <&hdmirx 1> still reaches the S/PDIF
DAI and gets the intended -EOPNOTSUPP. But you're right that the stub
is unnecessary and would wrongly force DAI 0 for an OF-graph card, so
I'll drop it in the next revision.

Igor

On Wed, Jul 15, 2026 at 10:22 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Severe logical and mathematical errors in Audio Clock Regeneration (ACR) N and CTS parsing, leading to hardware clock recovery failure.
> - [High] Dummy `hdmirx_audio_get_dai_id` callback unconditionally returns 0, breaking S/PDIF DAI OF graph mapping.
> --
>
> --- Patch [2]: [PATCH v2 2/2] media: synopsys: hdmirx: add HDMI audio capture support ---
> commit c4b60dd6e2a0b110adfab2b5a2ccf3d214ed0d05
> 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, so that a
> simple-audio-card in the device tree can bind the HDMI RX audio DAI.
>
> > diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > index 25f8ca0d6d946..acabd43664c10 100644
> > --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> > +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
>
> [ ... ]
>
> > @@ -2646,6 +2662,255 @@ static int hdmirx_register_cec(struct snps_hdmirx_dev *hdmirx_dev,
>
> [ ... ]
>
> > +/* 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((__force __be32)acr_pb7_4) & 0xfffff;
> > +     acr_n = (be32_to_cpu((__force __be32)acr_pb3_0) & 0x0fffff00) >> 8;
>
> [Severity: High]
> Does this code incorrectly parse the ACR N and CTS values?
>
> The variables acr_pb7_4 and acr_pb3_0 are assigned the swapped register
> contents (PKTDEC_ACR_PB3_0 is assigned to acr_pb7_4, and vice versa).
>
> Additionally, applying be32_to_cpu() to the native-endian result of
> hdmirx_readl() will cause big-endian byte-swapping on little-endian
> architectures. Because of the swapped assignment and byte reversal, the
> bitwise shift operations extract completely incorrect bytes.
>
> Will this cause hardware clock regeneration to fail and silently fall back
> to the ALSA default sample rate (48kHz)?
>
> > +     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;
> > +}
>
> [ ... ]
>
> > +static int hdmirx_audio_get_dai_id(struct snd_soc_component *component,
> > +                                struct device_node *endpoint,
> > +                                void *data)
> > +{
> > +     return 0;
> > +}
>
> [Severity: High]
> Does this dummy callback break the S/PDIF DAI OF graph mapping?
>
> The driver registers two audio DAIs (0 for I2S, 1 for S/PDIF) to match the
> DT binding. However, this callback unconditionally returns 0, which overrides
> the standard DAI index parsing in the ASoC core.
>
> Consequently, any DT endpoint requesting S/PDIF (reg = <1>) will be
> incorrectly forced to map to DAI 0 (I2S). This bypasses the intended
> -EOPNOTSUPP rejection in hdmirx_audio_hw_params() and erroneously configures
> the hardware for I2S when S/PDIF was requested.
>
> Could this dummy callback simply be removed to allow the core to map the reg
> property to the correct DAI ID naturally?
>
> > +
> > +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,
> > +};
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260715200834.8486-1-royalnet026@gmail.com?part=2