Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3] arm64: dts: rockchip: add SPDIF node for
 rk3399-rockpro64
To:     Katsuhiro Suzuki <katsuhiro@katsuster.net>,
        Heiko Stuebner <heiko@sntech.de>,
        linux-rockchip@lists.infradead.org
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20201005140311.2507530-1-katsuhiro@katsuster.net>
From:   Johan Jonker <jbx6244@gmail.com>
Message-ID: <e5ab2c62-ad00-4cdf-8b0a-24fda59c980b@gmail.com>
Date:   Tue, 6 Oct 2020 11:51:58 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20201005140311.2507530-1-katsuhiro@katsuster.net>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Katsuhiro, Heiko,

Question for the maintainer:
Should we add a SPDIF node if the connector is not physical on a board,
only a header?

Thanks Katsuhiro for the "aplay -l" screen print.

**** List of PLAYBACK Hardware Devices ****
card 0: hdmisound [hdmi-sound], device 0: ff8a0000.i2s-i2s-hifi
i2s-hifi-0 [ff8a0000.i2s-i2s-hifi i2s-hifi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 1: rockchiprk3399 [rockchip,rk3399], device 0: ff890000.i2s-ES8316
HiFi ES8316 HiFi-0 [ff890000.i2s-ES8316 HiFi ES8316 HiFi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0
card 2: rockchiprk339_1 [rockchip,rk3399], device 0:
ff870000.spdif-dit-hifi dit-hifi-0 [ff870000.spdif-dit-hifi dit-hifi-0]
  Subdevices: 1/1
  Subdevice #0: subdevice #0


On 10/5/20 4:03 PM, Katsuhiro Suzuki wrote:
> This patch adds 'disabled' SPDIF sound node and related settings
> of SPDIF for rk3399-rockpro64.
> 
> RockPro64 has output pins for SPDIF Tx. But RK3399 does not have
> enough DMA channel for enabling SPDIF tx. Current settings are:
> 
>   - I2S0     (Req num 0, 1): Enabled : Output to 40pins header CON40
>   - I2S1     (Req num 2, 3): Enabled : Output to ES8316 on board
>   - I2S2     (Req num 4, 5): Enabled : Output to internal HDMI core
>   - SPDIF Tx (Req num 7)   : Disabled: Output to connector J10
> 
> If users want to enable ALL sound I/Os, we need 7 DMA channels for
> it. But unfortunately, RK3399 has only 6 DMA channels. So users have
> to choose from the following:
> 
>   - Disable one of I2S (Ex. I2S0) and enable SPDIF tx
>   - Keep enable I2S0/1/2 and disable SPDIF tx
> 
> Signed-off-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> 
> ---
> 
> Changes in v3:
>   - Refine commit description why adding disabled node
> 
> Changes in v2:
>   - Remove redundant status property
> ---
>  .../boot/dts/rockchip/rk3399-rockpro64.dtsi   | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> index 6e553ff47534..58097245994a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> @@ -76,6 +76,23 @@ sound {
>  		dais = <&i2s1_p0>;
>  	};
>  


	hdmi_sound: hdmi-sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "hdmi-sound";

Maybe rename to "HDMI"?

[..]
	};

	sound {
		compatible = "audio-graph-card";
		label = "rockchip,rk3399";

Maybe change this to "ES8316" to prevent confusion?

		dais = <&i2s1_p0>;
	};


> +	sound-dit {
> +		compatible = "audio-graph-card"
> +		label = "rockchip,rk3399";

This would be the second sound card with the same label.
It seems that aplay/linux? adds "-1" to it and removes the comma, so we get:

hdmisound
rockchiprk3399
rockchiprk339_1

Shouldn't we label it with something that reflect the function/output.
Shouldn't we standardize to SPDIF, HDMI and Analog similar to rk3318/rk3328?
Make a shorter label without spaces or special chars, so that chars
don't get removed?

Proposal:

HDMI
ES8316
SPDIF

> +		dais = <&spdif_p0>;

Maybe disable too?

The "sound-dit" node is standard enabled and will start some process
with a dia in a node that is disabled.


> +	};
> +
> +	spdif-dit {
> +		compatible = "linux,spdif-dit";
> +		#sound-dai-cells = <0>;

Maybe disable too?

> +
> +		port {
> +			dit_p0_0: endpoint {

> +				remote-endpoint = <&spdif_p0_0>;

This also points to something that's disabled.

> +			};
> +		};
> +	};
> +
>  	vcc12v_dcin: vcc12v-dcin {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc12v_dcin";
> @@ -698,6 +715,16 @@ &sdhci {
>  	status = "okay";
>  };
>  
> +&spdif {
> +	pinctrl-0 = <&spdif_bus_1>;

This node is disabled.

> +
> +	spdif_p0: port {
> +		spdif_p0_0: endpoint {
> +			remote-endpoint = <&dit_p0_0>;
> +		};
> +	};
> +};
> +
>  &spi1 {
>  	status = "okay";
>  
>