Re: [PATCH v8 5/5] arm64: dts: amlogic: Add Amlogic S4 Audio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] arm64: dts: amlogic: wrong spdifin interrupt number in
  meson-s4.dtsi
- [Medium] arm64: dts: amlogic: missing pinctrl configuration for
  spdifin on AQ222
--

commit ba863296d8a1f9c2893c9c4c64db87474f58e6e7
Author: Jiebing Chen <jiebing.chen@amlogic.com>

arm64: dts: amlogic: Add Amlogic S4 Audio

This commit adds audio controller device tree nodes for the Amlogic S4 SoC
and enables the sound card and audio peripherals on the AQ222 board.

> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dts b/arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dts
> index 0a3f81ea0fb0a..334b57bde4def 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dts

[ ... ]

> @@ -129,6 +140,207 @@ vddcpu: regulator-vddcpu {
>  				<699000 98>,
>  				<689000 100>;
>  	};

[ ... ]

> +		/* spdif coax input */
> +		dai-link-11 {
> +			sound-dai = <&spdifin>;
> +
> +			codec {
> +				sound-dai = <&spdif_dir>;
> +			};
> +		};

[ ... ]

> @@ -235,3 +447,102 @@ &ethmac {
>  	phy-handle = <&internal_ephy>;
>  	phy-mode = "rmii";
>  };

[ ... ]

> +&spdifin {
> +	status = "okay";
> +};
> +
> +&spdifout_a {
> +	pinctrl-0 = <&spdif_out_h_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};

[Severity: Medium]
Is &spdifin missing pinctrl-0 and pinctrl-names properties to mux the
spdif_in pin (and a corresponding spdif_in pinmux node in meson-s4.dtsi)?

dai-link-11 wires &spdifin to the external coaxial receiver &spdif_dir,
and pinctrl-meson-s4.c defines the spdif_in function on the pad next to
spdif_out_h, but unlike &spdifout_a, &spdifin has no pinctrl configuration
and will not receive an external input signal unless the pin was already
muxed by firmware.

> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> index dfc0a30a6e61b..98ac01c5d6adb 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi

[ ... ]

> @@ -863,4 +878,385 @@ emmc: mmc@fe08c000 {

[ ... ]

> +	toddr_a: audio-controller@330100 {
> +		compatible = "amlogic,sm1-toddr",
> +				 "amlogic,axg-toddr";
> +		reg = <0x0 0x330100 0x0 0x2c>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "TODDR_A";
> +		interrupts = <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&clkc_audio AUD_CLKID_TODDR_A>;
> +		resets = <&arb AXG_ARB_TODDR_A>,
> +			 <&clkc_audio AUD_RESET_TODDR_A>;
> +		reset-names = "arb", "rst";
> +		amlogic,fifo-depth = <8192>;
> +		status = "disabled";
> +	};
> +
> +	toddr_b: audio-controller@330140 {
> +		compatible = "amlogic,sm1-toddr",
> +				 "amlogic,axg-toddr";
> +		reg = <0x0 0x330140 0x0 0x2c>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "TODDR_B";
> +		interrupts = <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&clkc_audio AUD_CLKID_TODDR_B>;
> +		resets = <&arb AXG_ARB_TODDR_B>,
> +			 <&clkc_audio AUD_RESET_TODDR_B>;
> +		reset-names = "arb", "rst";
> +		amlogic,fifo-depth = <256>;
> +		status = "disabled";
> +	};
> +
> +	toddr_c: audio-controller@330180 {
> +		compatible = "amlogic,sm1-toddr",
> +				 "amlogic,axg-toddr";
> +		reg = <0x0 0x330180 0x0 0x2c>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "TODDR_C";
> +		interrupts = <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&clkc_audio AUD_CLKID_TODDR_C>;
> +		resets = <&arb AXG_ARB_TODDR_C>,
> +			 <&clkc_audio AUD_RESET_TODDR_C>;
> +		reset-names = "arb", "rst";
> +		amlogic,fifo-depth = <256>;
> +		status = "disabled";
> +	};
> +
> +	frddr_a: audio-controller@3301c0 {
> +		compatible = "amlogic,sm1-frddr",
> +				 "amlogic,axg-frddr";
> +		reg = <0x0 0x3301c0 0x0 0x2c>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "FRDDR_A";
> +		interrupts = <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>;

[ ... ]

> +	spdifin: audio-controller@330400 {
> +		compatible = "amlogic,g12a-spdifin",
> +				 "amlogic,axg-spdifin";
> +		reg = <0x0 0x330400 0x0 0x30>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "SPDIFIN";
> +		interrupts = <GIC_SPI 151 IRQ_TYPE_EDGE_RISING>;
> +		clocks = <&clkc_audio AUD_CLKID_SPDIFIN>,
> +		<&clkc_audio AUD_CLKID_SPDIFIN_CLK>;
> +		clock-names = "pclk", "refclk";
> +		resets = <&clkc_audio AUD_RESET_SPDIFIN>;
> +		status = "disabled";
> +	};

[Severity: Medium]
Should the spdifin interrupt be 35 instead of 151?

On Amlogic SoCs (such as meson-g12.dtsi and meson-sm1.dtsi, which use
148..154), the interrupts for toddr_a..toddr_c, spdifin, and
frddr_a..frddr_c form a contiguous block.  Here toddr_a..toddr_c are at
32..34 and frddr_a..frddr_c are at 36..38, while spdifin still has 151
from G12A/SM1, so any consumer of the spdifin interrupt will bind to the
wrong line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-audio_drvier-v8-0-1489241e4c83@amlogic.com?part=5