Re: [PATCH v3 2/2] arm64: dts: rockchip: Add HDMI0 audio output on rock-5b

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Quentin,

On Friday, 31 January 2025 11:38:34 EST Quentin Schulz wrote:
> Hi Detlev,
> 
> On 1/30/25 5:45 PM, Detlev Casanova wrote:
> > Use the simple-audio-card driver with the hdmi0 QP node as CODEC and
> > the i2s5 device as CPU.
> > 
> > The simple-audio-card,mclk-fs value is set to 128 as it is done in
> > the downstream driver.
> > 
> > The #sound-dai-cells value is set to 0 in the hdmi0 node so that it can be
> > used as an audio codec node.
> > 
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  1 +
> >   .../boot/dts/rockchip/rk3588-rock-5b.dts      | 19 +++++++++++++++++++
> >   2 files changed, 20 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi index
> > 8cfa30837ce72..790c1c25a3e41 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> > @@ -1394,6 +1394,7 @@ hdmi0: hdmi@fde80000 {
> > 
> >   		reset-names = "ref", "hdp";
> >   		rockchip,grf = <&sys_grf>;
> >   		rockchip,vo-grf = <&vo1_grf>;
> > 
> > +		#sound-dai-cells = <0>;
> > 
> >   		status = "disabled";
> >   		
> >   		ports {
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts index
> > d597112f1d5b8..1909078538367 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -49,6 +49,21 @@ hdmi0_con_in: endpoint {
> > 
> >   		};
> >   	
> >   	};
> > 
> > +	hdmi0-sound {
> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,mclk-fs = <128>;
> > +		simple-audio-card,name = "hdmi0";
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&hdmi0>;
> > +		};
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s5_8ch>;
> > +		};
> > +	};
> > +
> 
> This is SoC specific and not board specific I believe. I2S5 is not
> usable anywhere else but with HDMI0 controller if I've read the TRM
> properly.
> 
> Therefore, I would suggest to move this to rk3588-base.dtsi (with
> status=disabled; and when hdmi1-sound comes up, to rk3588-extra.dtsi),
> the same way we've done for RK3399 for example.

Indeed, I only could test on a Rock 5B, but I think it can be moved to the SoC 
DTS.

> The only hesitation I have is that HDMI0 can use either I2S or SPDIF for
> audio, both audio controllers internal exclusive to HDMI0/1 controller.
> But the user could anyway define their own simple audio card for spdif
> or modify this one if they wanted to.

So some boards will use I2S and some SPDIF ? Or any board can be used with one 
or the other ?
The disabled status makes sense as hdmi is disabled in the SoC tree as well. 
So if a user wants to use SPDIF instead, they could keep hdmi0-sound disabled 
and add their own simple-audio-card compatible node.

> I've tested with my suggested change and same changes than for Rock 5B
> DTS on RK3588 Tiger Haikou with speaker-test -c 2 -t wav, left is left,
> right is right :)
> 
> I'm not giving my Tb here as the commit title is specifically about Rock
> 5B and I haven't tested this series on it. If you had a separate patch
> for the hdmi sound node and enabling it on Rock 5b, I would have given
> my Tb on the former and not the latter.

Thank you for testing anyway ! I will move the node and enable it in all board 
dts that already enable hdmi0.


Detlev.