Re: [PATCH v1 3/3] riscv: dts: starfive: add tdm node and sound card

On 29/03/2023 17:33, Walker Chen wrote:

Add the tdm controller node and sound card for the StarFive JH7110 SoC.

Signed-off-by: Walker Chen walker.chen@starfivetech.com

.../jh7110-starfive-visionfive-2.dtsi | 87 +++++++++++++++++++ arch/riscv/boot/dts/starfive/jh7110.dtsi | 34 ++++++++ 2 files changed, 121 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi index 1155b97b593d..35137c2edf5d 100644 --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi @@ -62,6 +62,10 @@ clock-frequency = <297000000>; };

+&wm8960_mclk {

• clock-frequency = <24576000>;

+};

&i2srx_bclk_ext { clock-frequency = <12288000>; }; @@ -102,6 +106,14 @@ pinctrl-names = "default"; pinctrl-0 = <&i2c0_pins>; status = "okay";

• wm8960: codec@1a {

• compatible = "wlf,wm8960";
  

• reg = <0x1a>;
  

• #sound-dai-cells = <0>;
  

• wlf,shared-lrclk;
  

• };

};

&i2c2 { @@ -214,6 +226,40 @@ slew-rate = <0>; }; };

• tdm0_pins: tdm0-pins {

• tdm0-pins-tx {
  

• 	pinmux = <GPIOMUX(44, GPOUT_SYS_TDM_TXD,
  

• 			      GPOEN_ENABLE,
  

• 			      GPI_NONE)>;
  

• 	bias-pull-up;
  

• 	drive-strength = <2>;
  

• 	input-disable;
  

• 	input-schmitt-disable;
  

• 	slew-rate = <0>;
  

• };
  

• tdm0-pins-rx {
  

• 	pinmux = <GPIOMUX(61, GPOUT_HIGH,
  

• 			      GPOEN_DISABLE,
  

• 			      GPI_SYS_TDM_RXD)>;
  

• 	input-enable;
  

• };
  

• tdm0-pins-sync {
  

• 	pinmux = <GPIOMUX(63, GPOUT_HIGH,
  

• 			      GPOEN_DISABLE,
  

• 			      GPI_SYS_TDM_SYNC)>;
  

• 	input-enable;
  

• };
  

• tdm0-pins-pcmclk {
  

• 	pinmux = <GPIOMUX(38, GPOUT_HIGH,
  

• 			      GPOEN_DISABLE,
  

• 			      GPI_SYS_TDM_CLK)>;
  

• 	input-enable;
  

• };
  

• };

};

&uart0 { @@ -221,3 +267,44 @@ pinctrl-0 = <&uart0_pins>; status = "okay"; };

+&tdm {

• pinctrl-names = "default";
• pinctrl-0 = <&tdm0_pins>;
• status = "okay";

+};

+&sound0 {

• simple-audio-card,dai-link@0 {

• reg = <0>;
  

• status = "okay";
  

Why? Drop.

• format = "dsp_a";
  

• bitclock-master = <&dailink_master>;
  

• frame-master = <&dailink_master>;
  

• widgets =
  

Drop line break.

• 		"Microphone", "Mic Jack",
  

• 		"Line", "Line In",
  

• 		"Line", "Line Out",
  

• 		"Speaker", "Speaker",
  

• 		"Headphone", "Headphone Jack";
  

• routing =
  

Drop unnecessary line break.

• 		"Headphone Jack", "HP_L",
  

• 		"Headphone Jack", "HP_R",
  

• 		"Speaker", "SPK_LP",
  

• 		"Speaker", "SPK_LN",
  

• 		"LINPUT1", "Mic Jack",
  

• 		"LINPUT3", "Mic Jack",
  

• 		"RINPUT1", "Mic Jack",
  

• 		"RINPUT2", "Mic Jack";
  

• cpu {
  

• 	sound-dai = <&tdm>;
  

• };
  

• dailink_master:codec {
  

Missing space after label:.

• 	sound-dai = <&wm8960>;
  

• 	clocks = <&wm8960_mclk>;
  

• 	clock-names = "mclk";
  

• };
  

• };

+}; diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi index b503b6137743..a89158d1d7a6 100644 --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi @@ -210,6 +210,12 @@ #clock-cells = <0>; };

• wm8960_mclk: wm8960_mclk {

No underscores in node names. Use consistent naming - do you see here any nodes named "mclk"?

Anyway this is some fake clock. Real clock should come out from wm8960.

• compatible = "fixed-clock";
  

• clock-output-names = "wm8960_mclk";
  

• #clock-cells = <0>;
  

• };
• i2srx_bclk_ext: i2srx-bclk-ext-clock { compatible = "fixed-clock"; clock-output-names = "i2srx_bclk_ext";

@@ -375,6 +381,27 @@ status = "disabled"; };

• tdm: tdm@10090000 {
  

• 	compatible = "starfive,jh7110-tdm";
  

• 	reg = <0x0 0x10090000 0x0 0x1000>;
  

• 	clocks = <&syscrg JH7110_SYSCLK_TDM_AHB>,
  

• 		 <&syscrg JH7110_SYSCLK_TDM_APB>,
  

• 		 <&syscrg JH7110_SYSCLK_TDM_INTERNAL>,
  

• 		 <&syscrg JH7110_SYSCLK_TDM_TDM>,
  

• 		 <&syscrg JH7110_SYSCLK_MCLK_INNER>,
  

• 		 <&tdm_ext>;
  

• 	clock-names = "tdm_ahb", "tdm_apb",
  

• 		      "tdm_internal", "tdm",
  

• 		      "mclk_inner", "tdm_ext";
  

• 	resets = <&syscrg JH7110_SYSRST_TDM_AHB>,
  

• 		 <&syscrg JH7110_SYSRST_TDM_APB>,
  

• 		 <&syscrg JH7110_SYSRST_TDM_CORE>;
  

• 	dmas = <&dma 20>, <&dma 21>;
  

• 	dma-names = "rx","tx";
  

• 	#sound-dai-cells = <0>;
  

• 	status = "disabled";
  

• };
  

• stgcrg: clock-controller@10230000 { compatible = "starfive,jh7110-stgcrg"; reg = <0x0 0x10230000 0x0 0x10000>;

@@ -601,5 +628,12 @@ #reset-cells = <1>; power-domains = <&pwrc JH7110_PD_VOUT>; };

• sound0: snd-card0 {
  

1. Why card0? 2. Where is this node located? In MMIO bus? Run some basic checks on your DTS before submitting upstream. dtbs_check dtbs W=1

3. Why this is even in the DTSI? This really looks wrong.

• 	compatible = "simple-audio-card";
  

• 	simple-audio-card,name = "Starfive-TDM-Sound-Card";
  

• 	#address-cells = <1>;
  

• 	#size-cells = <0>;
  

Best regards, Krzysztof