Re: [PATCH v1 3/5] arm64: dts: ti: add verdin am62

["Raghavendra, Vignesh" <vigneshr@ti.com>]

Hi

On 5/30/2023 5:40 PM, Nishanth Menon wrote:
> 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
> 

Checkpatch now seems outdated in favor of dtbs_check. DT schemas use
regex for compatibles and thus simple grep in
Doucmentation/devicetree-bindings/ doesn't help. Eg: In this case:

Documentation/devicetree/bindings/eeprom/at24.yaml has the regex to
cover the compatibles

>> +		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).

Same here ;)
Documentation/devicetree/bindings/net/ethernet-phy.yaml covers it

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