Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 07:10:44 -0500
From:   Nishanth Menon <nm@ti.com>
To:     Francesco Dolcini <francesco@dolcini.it>
CC:     Vignesh Raghavendra <vigneshr@ti.com>,
        Francesco Dolcini <francesco.dolcini@toradex.com>,
        Tero Kristo <kristo@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 3/5] arm64: dts: ti: add verdin am62
Message-ID: <20230530121044.sjhv452b4hs4lyiy@flyer>
References: <20230524143631.42471-1-francesco@dolcini.it>
 <20230524143631.42471-4-francesco@dolcini.it>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230524143631.42471-4-francesco@dolcini.it>
Precedence: bulk

On 16:36-20230524, Francesco Dolcini wrote:
[...]
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin-dev.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin-dev.dtsi
> new file mode 100644
> index 000000000000..e138b1c8ed14
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin-dev.dtsi
> @@ -0,0 +1,233 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> +/*
> + * Copyright 2023 Toradex

Please also add appropriate product links to dts/dtsi

> + */
> +
> +/ {
> +	sound_card: sound-card {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,bitclock-master = <&dailink_master>;
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,frame-master = <&dailink_master>;
> +		simple-audio-card,name = "verdin-nau8822";
> +		simple-audio-card,routing =
> +			"Headphones", "LHP",
> +			"Headphones", "RHP",
> +			"Speaker", "LSPK",
> +			"Speaker", "RSPK",
> +			"Line Out", "AUXOUT1",
> +			"Line Out", "AUXOUT2",
> +			"LAUX", "Line In",
> +			"RAUX", "Line In",
> +			"LMICP", "Mic In",
> +			"RMICP", "Mic In";
> +		simple-audio-card,widgets =
> +			"Headphones", "Headphones",
> +			"Line Out", "Line Out",
> +			"Speaker", "Speaker",
> +			"Microphone", "Mic In",
> +			"Line", "Line In";
> +
> +		dailink_master: simple-audio-card,codec {
> +			clocks = <&k3_clks 157 10>;
> +			sound-dai = <&nau8822_1a>;
> +		};
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&mcasp0>;
> +		};
> +	};
> +};
> +
> +/* Verdin ETHs */
> +&cpsw3g {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rgmii1 &pinctrl_rgmii2>;

here and elsewhere:
pinctrl-0 = <&pinctrl_rgmii1>, <&pinctrl_rgmii2>;


> +	status = "okay";
> +};
[...]
> +	/* EEPROM */
> +	eeprom@57 {
> +		compatible = "st,24c02", "atmel,24c02";

checkpatch warns: DT compatible string "st,24c02" appears un-documented

> +		reg = <0x57>;
> +		pagesize = <16>;
> +	};
> +};
> +
> +/* Verdin I2C_2_DSI */
> +&main_i2c2 {
> +	status = "okay";

Here and few other dtsis:
you should set status along with pinmux.

[...]
> +
> +/* Verdin UART_2 */
> +&wkup_uart0 {
> +	/* FIXME: WKUP UART0 is used by DM firmware */
> +	status = "reserved";

If you do configure this in R5 SPL, you'd want to add the pinmux as
well.

> +};
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin-wifi.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin-wifi.dtsi
> new file mode 100644
> index 000000000000..289db1666fc0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin-wifi.dtsi
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> +/*
> + * Copyright 2023 Toradex
> + */
> +
> +/ {
> +	wifi_pwrseq: wifi-pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_wifi_en>;
> +		reset-gpios = <&main_gpio0 22 GPIO_ACTIVE_LOW>;
> +	};
> +};
> +
> +

Drop extra EoLs.

[...]

> diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> new file mode 100644
> index 000000000000..2e7cb607df45
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> @@ -0,0 +1,1400 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT

Assuming you really intent this instead of purely GPL-2.0

[...]

> +/* MDIO, shared by Verdin ETH_1 (On-module PHY) and Verdin ETH_2_RGMII */
> +&cpsw3g_mdio {
> +	assigned-clocks = <&k3_clks 157 20>;
> +	assigned-clock-parents = <&k3_clks 157 22>;
> +	assigned-clock-rates = <25000000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_eth_clock &pinctrl_mdio>;
> +	status = "disabled";
> +
> +	cpsw3g_phy0: ethernet-phy@0 {
> +		compatible = "ethernet-phy-id2000.a231";

Check binding - we don't include any compatibles that dont have yaml
conversion done (pinctrl is the only exception).

[...]

> +/* TODO: Verdin DSI_1 / TIDSS */
Drop the TODO.

[...]

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D