Re: [PATCH v3 4/4] arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The audio_work delayed work is not cancelled during system suspend, leading to a fatal bus fault.
- [Medium] Incorrect use of be32_to_cpu() for unconditional byte swapping breaks audio capture on big-endian architectures.
--

--- Patch [4]: [PATCH v3 4/4] arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus ---
commit 030cd8fd5334b79e1751dec57b1de1f5fa4e3df3
Author: Igor Paunovic <royalnet026@gmail.com>

arm64: dts: rockchip: enable HDMI RX audio capture on Orange Pi 5 Plus

This patch routes the HDMI receiver audio to the i2s7_8ch interface. It creates
a simple-audio-card configuration to expose a capture-only ALSA card driven
by the HDMI input. The setup uses the receiver as the bitclock and frame master.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> index 145986695720a..2a68610cddbc3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-orangepi-5-plus.dts
> @@ -37,6 +37,22 @@ hdmi1_con_in: endpoint {
>  		};
>  	};
>  
> +	hdmiin-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,name = "rockchip,hdmiin";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,bitclock-master = <&hdmiin_codec>;
> +		simple-audio-card,frame-master = <&hdmiin_codec>;
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s7_8ch>;
> +		};
> +
> +		hdmiin_codec: simple-audio-card,codec {
> +			sound-dai = <&hdmi_receiver 0>;

[Severity: High]
Does exposing this codec to the ALSA subsystem trigger a suspend regression in
the hdmirx driver?

Looking at the underlying codec driver hdmirx_suspend() in snps_hdmirx.c:

drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c:hdmirx_suspend()
{
	struct snps_hdmirx_dev *hdmirx_dev = dev_get_drvdata(dev);

	hdmirx_disable_irq(dev);

	/* TODO store CEC HW state */
	disable_irq(hdmirx_dev->cec->irq);

	return hdmirx_disable(dev);
}

The audio_work delayed work is queued on system_unbound_wq, which is not
freezable, but this suspend callback doesn't cancel it. 

If system suspend is initiated while audio capture is active, the worker
could fire after clocks are disabled via hdmirx_disable(), resulting in
a bus fault when it performs MMIO reads.

[Severity: Medium]
Will this audio capture work correctly on big-endian architectures?

The underlying hdmirx_audio_fs() driver function uses be32_to_cpu() for an
unconditional byte swap when calculating the ACR subpacket:

drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c:hdmirx_audio_fs()
{
...
	acr_cts = be32_to_cpu((__force __be32)acr_pb3_0) & 0xfffff;
	acr_n = (be32_to_cpu((__force __be32)acr_pb7_4) & 0x0fffff00) >> 8;
...
}

Since be32_to_cpu() behaves as a no-op on big-endian CPUs, the bytes remain
unswapped, causing the bitwise masking to extract the wrong bytes for the
ACR CTS and N fields. 

Should swab32() be used here instead to ensure the byte swap happens
unconditionally on all architectures?

> +		};
> +	};
> +

[ ... ]

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