| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <693c38f8-6aec-dd50-94a0-65300b6ed1b2@starfivetech.com>
Date: Mon, 3 Apr 2023 20:58:54 +0800
From: Walker Chen <walker.chen@...rfivetech.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor.dooley@...rochip.com>,
Emil Renner Berthing <emil.renner.berthing@...onical.com>
CC: <alsa-devel@...a-project.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH v1 3/3] riscv: dts: starfive: add tdm node and sound card
On 2023/3/30 15:43, Krzysztof Kozlowski wrote:
> 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@...rfivetech.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.
Will drop it.
>
>> + format = "dsp_a";
>> + bitclock-master = <&dailink_master>;
>> + frame-master = <&dailink_master>;
>> +
>> + widgets =
>
> Drop line break.
OK, will drop it.
>
>> + "Microphone", "Mic Jack",
>> + "Line", "Line In",
>> + "Line", "Line Out",
>> + "Speaker", "Speaker",
>> + "Headphone", "Headphone Jack";
>> + routing =
>
> Drop unnecessary line break.
OK, will drop it.
>
>> + "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:.
Will be fixed.
>
>> + 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.
Thank you for pointing out this, it will be fixed.
>
>> + 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@...90000 {
>> + 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@...30000 {
>> 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?
There are several audio interfaces in JH7110 SoC, each as an independent sound card.
TDM is for snd-card0, latter i2s will be for snd-card1, spdif will be for snd-card2, etc.
> 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.
It seems that the sound node should be located in DTS file more appropriately.
Best regards,
Walker
Powered by blists - more mailing lists